Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test(integration): Use node for running test scripts #167

Merged
merged 3 commits into from Jan 28, 2019

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Jan 8, 2019

This adds @rpl’s changes from #163 that convert the bash scripts to node scripts, making testing easier on Windows (the platform I use for development).

review?(@Rob--W, @rpl)

@Rob--W
Copy link
Member

Rob--W commented Jan 8, 2019

I haven't reviewed the PR in detail, but I approve of the motivation behind it.
What are the high-level differences between your PR and the original? I see that some of my requested changes (e.g. the pipefail one) has not been addressed in the first commit, but they have been addressed separately in the second commit.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Jan 8, 2019

The primary difference is the removal of Edge‑specific code.

Then there’s the ESLint configuration improvements and better Windows compatibility.

Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

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

@ExE-Boss Thank you so much! Follows a round of review comments.

src/browser-polyfill.js Show resolved Hide resolved
scripts/run-browsers-smoketests.js Outdated Show resolved Hide resolved
scripts/run-browsers-smoketests.js Show resolved Hide resolved
scripts/run-module-bundlers-smoketests.js Outdated Show resolved Hide resolved
scripts/run-module-bundlers-smoketests.js Show resolved Hide resolved
for (const extensionDirName of extensions) {
test(`${description} (test extension: ${extensionDirName})`, async (tt) => {
let timeout;
let driver;
let server;
let tempDir;

const browser = process.env.TEST_BROWSER_TYPE;

if (skip) {
Copy link
Member

Choose a reason for hiding this comment

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

By removing from #163 the pieces related to Edge, there is no use of the skip property, and so we may choose to defer it or remove it as @Rob--W suggested in #163.

Copy link
Member

@rpl rpl left a comment

Choose a reason for hiding this comment

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

@ExE-Boss Thanks, this looks good to me, "configuring eslint to allow just module as a valid global" sounds like a reasonable enough approach, as an alternative to disable the eslint rule on those two lines.

I would like to apply the following small additional changes to the .eslintrc files (now that we have two specific ones in the src and test directories):

  • remove browser and webextensions envs from the top level .eslintrc
  • add browser and webextensions envs to src/.eslintrc and test/.eslintrc

But it is just a small optional nit, and so I'm going to merge this as it is and apply these two small changes in a follow up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants