Skip to content

Refactor: move migration script test out of linter#9230

Merged
queengooborg merged 3 commits intomdn:mainfrom
bershanskiy:linter-refactor-testMigrations
Apr 19, 2022
Merged

Refactor: move migration script test out of linter#9230
queengooborg merged 3 commits intomdn:mainfrom
bershanskiy:linter-refactor-testMigrations

Conversation

@bershanskiy
Copy link
Contributor

@bershanskiy bershanskiy commented Feb 23, 2021

This PR is very similar to #9229, but it refactors a different function, testMigrations().

This PR moves a unit test out of linter and into a separate mocha test file similar to utils.test.js. This unit test is still called by npm run test, since test script includes npm run mocha.

For reviewer convenience, I split it up into two commits: the actual change is in the first commit and the second commit is just npm run fix.

Running npm run test

Before:

$ npm run test

> @mdn/[email protected] test
> npm run mocha && npm run lint


> @mdn/[email protected] mocha
> mocha "test/**.test.js"



  utils
    √ `escapeInvisibles()` works correctly


  1 passing (3ms)


> @mdn/[email protected] lint
> node test/lint
> [rest of output]

After:

$ npm run test

> @mdn/[email protected] test
> npm run mocha && npm run lint


> @mdn/[email protected] mocha
> mocha "**/**.test.js"



  migration scripts
    √ `removeWebViewFlags()` works correctly

  utils
    √ `escapeInvisibles()` works correctly


  2 passing (6ms)


> @mdn/[email protected] lint
> node test/lint
> [rest of output]

A checklist to help your pull request get merged faster:

  • Summarize your changes
  • Data: link to resources that verify support information (such as browser's docs, changelogs, source control, bug trackers, and tests)
  • Data: if you tested something, describe how you tested with details like browser and version
  • Review the results of the linter and fix problems reported (If you need help, please ask in a comment!)
  • Link to related issues or pull requests, if any

@bershanskiy bershanskiy requested a review from ddbeck as a code owner February 23, 2021 11:33
@github-actions github-actions bot added bulk_update An update to a mass amount of data, or scripts/linters related to such changes dependencies Pull requests that update a dependency package or file. infra Infrastructure issues (npm, GitHub Actions, releases) of this project linter Issues or pull requests regarding the tests / linter of the JSON files. scripts Issues or pull requests regarding the scripts in scripts/. labels Feb 23, 2021
@bershanskiy bershanskiy changed the title Linter refactor test migrations Refactor: move migration script test out of linter Feb 23, 2021
Base automatically changed from master to main March 24, 2021 12:54
@bershanskiy bershanskiy mentioned this pull request Apr 14, 2021
Merged
5 tasks
@bershanskiy
Copy link
Contributor Author

bershanskiy commented Apr 15, 2021

(Force-pushed after rebase.)

@bershanskiy bershanskiy marked this pull request as draft April 15, 2021 17:04
@bershanskiy
Copy link
Contributor Author

bershanskiy commented Jun 7, 2021

Just to clarify: this PR is OK as is, but I marked it as "Draft" because it is bound to merge conflict with #9229.

@bershanskiy
Copy link
Contributor Author

bershanskiy commented Dec 6, 2021

(Force-pushed after rebase.)

@bershanskiy
Copy link
Contributor Author

bershanskiy commented Dec 6, 2021

I think this will no longer have merge conflicts with #9229, so marking this as ready.

@bershanskiy bershanskiy marked this pull request as ready for review December 6, 2021 14:16
@queengooborg queengooborg removed the request for review from ddbeck April 18, 2022 15:00
queengooborg
queengooborg requested changes Apr 18, 2022
View reviewed changes
Copy link
Contributor

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm definitely down for script tests being moved out of lint.js -- thank you! Just got a little nit, but LGTM overall:

scripts/migrations/002-remove-webview-flags.test.js Outdated

if (output !== expected) {
logger.error(chalk`{red WebView flags aren't removed properly!}
if (output !== expected) {
Copy link
Contributor

@queengooborg queengooborg Apr 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we opt for using assert.deepStrictEqual(output, expected) here?

Copy link
Contributor Author

@bershanskiy bershanskiy Apr 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we elect to use assert.deepStrictEqual(output, expected) we can probably remove the whole JSON normalization (parsing and then stringification of data). Should I do that?

Copy link
Contributor

@queengooborg queengooborg Apr 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's actually a great point, we could simply write this as:

for (const test of tests) {
  assert.deepStrictEqual(output, expected);
}

Copy link
Contributor Author

@bershanskiy bershanskiy Apr 18, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used the following:

    for (const test of tests) {
      const expected = test.output;
      const output = JSON.parse(JSON.stringify(test.input), removeWebViewFlags);
      assert.deepStrictEqual(output, expected);
    }

@queengooborg queengooborg mentioned this pull request Apr 19, 2022
Closed
queengooborg
queengooborg approved these changes Apr 19, 2022
View reviewed changes
Copy link
Contributor

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@queengooborg
Copy link
Contributor

queengooborg commented Apr 19, 2022

Looks like your other PR conflicted with this one, so this PR needs a quick rebase!

queengooborg
queengooborg approved these changes Apr 19, 2022
View reviewed changes
Copy link
Contributor

@queengooborg queengooborg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@queengooborg queengooborg merged commit a4b6eed into mdn:main Apr 19, 2022
@bershanskiy bershanskiy deleted the linter-refactor-testMigrations branch April 19, 2022 13:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@queengooborg queengooborg queengooborg approved these changes

Assignees

No one assigned

Labels

bulk_update An update to a mass amount of data, or scripts/linters related to such changes dependencies Pull requests that update a dependency package or file. infra Infrastructure issues (npm, GitHub Actions, releases) of this project linter Issues or pull requests regarding the tests / linter of the JSON files. scripts Issues or pull requests regarding the scripts in scripts/.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@bershanskiy @queengooborg