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.

Sign up for GitHub

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

Jump to bottom

[BUG] rimraf causes failure with multiple browsers on Windows #808

Closed
jperl opened this issue Feb 2, 2020 · 8 comments · Fixed by #916
Closed

[BUG] rimraf causes failure with multiple browsers on Windows #808

jperl opened this issue Feb 2, 2020 · 8 comments · Fixed by #916

Comments

@jperl
Copy link
Contributor

jperl commented Feb 2, 2020 •
edited by aslushnikov
Loading

Context:

Might relate to #680

Code Snippet

Run npx jest

const { chromium } = require("playwright");

test("browser one success", async () => {
  const context = await chromium.launch();
  await context.close();
});

test("browser two failure", async () => {
  const context = await chromium.launch();
  await context.close();
});

Describe the bug

The second test throws this error.

assert(received)

    Expected value to be equal to:
      true
    Received:
      false

      at fixWinEPERM (node_modules/rimraf/rimraf.js:175:5)
      at node_modules/rimraf/rimraf.js:160:15

Others have seen this same issue with rimraf on windows.
jestjs/jest#3942 (comment)
GoogleChrome/lighthouse#2641

I tried to force the playwright-core resolution to rimraf 3.0.1 but I ran into the same issue.

Is there a way to remove the rimraf dependency and replace it with fs-extra, that seems to be an approach that worked here. getgauge/taiko#837
@jperl
Copy link
Contributor Author

jperl commented Feb 2, 2020

I am happy to submit a PR if you are willing to accept replacing rimraf with fs-extra.

@jperl jperl mentioned this issue Feb 2, 2020
Merged
18 tasks
@jperl jperl changed the title [BUG] rimraf causing issues for test on Windows [BUG] rimraf causes failure with multiple browsers on Windows Feb 2, 2020
@aslushnikov
Copy link
Contributor

aslushnikov commented Feb 4, 2020 •
edited
Loading

I am happy to submit a PR if you are willing to accept replacing rimraf with fs-extra.

@jperl If I understand correctly, fs-extra simply includes rimraf:

So I wonder how is this fixing the problem. I can reproduce this easily on win - interestingly, it only happens under jest environment, and never otherwise.

The assert that fires is very peculiar too. Let me see what's going on!

@aslushnikov
Copy link
Contributor

@jperl so rimraf turned out to have an assert that makes sure that errors reported by fs module are, well, actually errors:

https:/isaacs/rimraf/blob/d709272f6122b008de66650616fda34c8ae6f954/rimraf.js#L168

The assert fails because it looks like jest is messing up with fs errors somehow. I can repro this with the following jest test:

test('whoa', () => {
  try {
    require('fs').rmdirSync(__dirname);
  } catch (e) {
    console.log('caught error instanceof Error: ', e instanceof Error);;
  }
});

Running npx jest prints "caught error instance of Error: false" - on both Mac and Windows to me.

image

This is somewhat surprising since jest explicitly states that they don't mock core node modules.

Let me dig further.

@jperl
Copy link
Contributor Author

jperl commented Feb 4, 2020 •
edited
Loading

@aslushnikov Thank you for investigating this 🙏. I admittedly did not check that fs-extra solves this problem.

@aslushnikov
Copy link
Contributor

aslushnikov commented Feb 4, 2020 •
edited
Loading

Ok, we now know what's going on!

Investigation

  1. Jest runs tests in separate VMs (or execution contexts).
  2. For every VM, Jest creates a test environment: this defaults to JSDOMEnvironment. JSDOM's global is then used as VM's global.
  3. Test is executed in VM's global.

A reduced repro of this behavior looks like this:

const vm = require('vm');
const jsdom = require('jsdom');

const env = new jsdom.JSDOM('<!DOCTYPE html>', {
  runScripts: 'dangerously',
});

vm.createContext(env.window);

const testcode = `
  ((console) => {
    try {
      non_existent_variable_that_throws_not_defined_error;
    } catch (e) {
      console.log(e);
      console.log('caught error instanceof Error: ', e instanceof Error);;
    }
  })
`;
vm.runInContext(testcode, env.window)(console);
Reproduction without JSDOM 
const vm = require('vm');

const context = { Error, console };
vm.createContext(context);

const testcode = `
  try {
    non_existent_variable_that_throws_not_defined_error;
  } catch (e) {
    console.log(e);
    console.log('caught error instanceof Error: ', e instanceof Error);;
  }
`;
vm.runInContext(testcode, context);

This code prints "false" because:

  • the error in the global scope is coming from a parent execution context
  • the error that we catch is generated by native c++ code and thus created by the child execution context
  • these two errors are technically different in terms of instanceof'ing.

Now, rimraf is doing an instanceof check for some of the errors it's getting from FS module:

https:/isaacs/rimraf/blob/d709272f6122b008de66650616fda34c8ae6f954/rimraf.js#L168

This check fails and yields this error.

Mitigation

Unfortunately, configuring jest to run in a testEnvironment: node (and thus not bringing in JSDOM) doesn't help to mitigate this issue, because jest's NodeEnvironment is doing a very similar thing. (And there seem to be some workarounds for some similar problems)

Short-term, we should try fixing this upstream in rimraf - instead of instanceof'ing errors, it'd be probably fine to check for existence of a stack property.

Long-term, I wonder if there's something Jest can do better on their side to avoid this cross-vm type passing.

@jperl
Copy link
Contributor Author

jperl commented Feb 4, 2020

I'm impressed at how quickly you got to the root of this complex issue. Would you like me to submit the PR for rimraf?

@aslushnikov aslushnikov mentioned this issue Feb 4, 2020
Closed
@aslushnikov
Copy link
Contributor

@jperl I'm on it, thanks! 👷‍♂️

@aslushnikov
Copy link
Contributor

aslushnikov commented Feb 4, 2020 •
edited
Loading

Once/if isaacs/rimraf#209 is merged and a new version is released, we can roll the dependency. It'll fix this bug.

We'll probably wait for a week to see if we can land the fix to rimraf. If there's a delay, we can detect jest environment and provide rimraf with custom fs hooks that re-throw vm-native errors

const isJestEnvironment = (function() {
  try {
    some_undefined_variable_goes_here;
  } catch (e) {
    return !(e instanceof  Error);
  }
})();

aslushnikov added a commit to aslushnikov/playwright that referenced this issue Feb 10, 2020
@aslushnikov
chore: bump rimraf to 3.0.2
498f388 
Fixes microsoft#808
@aslushnikov aslushnikov mentioned this issue Feb 10, 2020
Merged
aslushnikov added a commit that referenced this issue Feb 10, 2020
@aslushnikov
chore: bump rimraf to 3.0.2 (#916)
9da0229 
Fixes #808
@yordan-kanchelov yordan-kanchelov mentioned this issue Feb 11, 2020
Closed
@aslushnikov aslushnikov mentioned this issue Feb 21, 2020
Merged
sand4rt pushed a commit to sand4rt/playwright that referenced this issue Dec 21, 2022
@playwrightmachine
feat(roll): roll to ToT Playwright (12-10-22) (microsoft#808)
3af1923
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

chore: bump rimraf to 3.0.2 aslushnikov/playwright
2 participants
@aslushnikov @jperl