search

nodejs / citgm

Canary in the Gold Mine
https://www.npmjs.com/package/citgm
Other
562 stars 147 forks source link

mark vinyl-fs as non-flaky #444

Open mcollina opened 7 years ago

mcollina commented 7 years ago

We recently discovered a regression on streams (https://github.com/nodejs/node/pull/13850) in the 8 line (and readable-stream 2.3.1), that we would have caught if this module was added.

What can we do to mark it as non-flaky anymore?

cc @phated

MylesBorins commented 7 years ago

We need to run tests and make sure it passes.

Will dig into this after I get back to computer

On Jun 21, 2017 2:22 PM, "Matteo Collina" notifications@github.com wrote:

We recently discovered a regression on streams (nodejs/node#13850 https://github.com/nodejs/node/pull/13850) in the 8 line (and readable-stream 2.3.1), that we would have caught if this module was added.

What can we do to mark it as non-flaky anymore?

cc @phated https://github.com/phated

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/nodejs/citgm/issues/444, or mute the thread https://github.com/notifications/unsubscribe-auth/AAecV3OSLDwbyfe8fVOo9RIP0-aZxlaDks5sGYmYgaJpZM4OBiTs .

phated commented 7 years ago

I have a vague memory of something thrashing with our setup/teardown because we have certain tests that rely on permissions

MylesBorins commented 7 years ago

Yeah, if the test suite is trying anything outside of the temp directory it is going to be no bueno. These executors are shared in ci

On Jun 21, 2017 2:33 PM, "Blaine Bublitz" notifications@github.com wrote:

I have a vague memory of something thrashing with our setup/teardown because we have certain tests that rely on permissions

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/nodejs/citgm/issues/444#issuecomment-310211690, or mute the thread https://github.com/notifications/unsubscribe-auth/AAecV8kkOwaa7J8ySMzz3MkJ4qM7l2ejks5sGYw0gaJpZM4OBiTs .

phated commented 7 years ago

We generate constants for all our paths in https://github.com/gulpjs/vinyl-fs/blob/master/test/utils/test-constants.js - maybe we can add a conditional to work with temp on citgm?

MylesBorins commented 7 years ago

What would be the benefit of not always doing this in a tempdir? I'm not sure how we could easily let you know the suite is running on citgm

On Jun 21, 2017 2:43 PM, "Blaine Bublitz" notifications@github.com wrote:

We generate constants for all our paths in https://github.com/gulpjs/ vinyl-fs/blob/master/test/utils/test-constants.js - maybe we can add a conditional to work with temp on citgm?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/nodejs/citgm/issues/444#issuecomment-310213884, or mute the thread https://github.com/notifications/unsubscribe-auth/AAecV-ncS4CnajpjRq36Mg4hknVxcBNVks5sGY6DgaJpZM4OBiTs .

gdams commented 7 years ago

@MylesBorins we can always use environment variables in the lookup.json?

gibfahn commented 7 years ago

Yeah, if the test suite is trying anything outside of the temp directory

We generate constants for all our paths in https://github.com/gulpjs/vinyl-fs/blob/master/test/utils/test-constants.js

I'm probably missing something, but all those paths look like they're relative paths pointing to folders inside the test directory. Which specific paths are the issue?

phated commented 7 years ago

It looks like there was also an issue with drifting timestamps but a user did further research and thinks it has something to do with node versions. See https://github.com/gulpjs/vinyl-fs/issues/208#issuecomment-310716961 for more info.

phated commented 7 years ago

The drifting timestamp thing should be fixed in our master. Guessing fs stuff still needs to be looked into?