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

Policy integrity test refactor via permutations #34404

Closed
wants to merge 1 commit into from

Conversation

bmeck
Copy link
Member

@bmeck bmeck commented Jul 17, 2020

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines

This refactors the policy test to do a full set of permutations of various possible configurations. It is rather extensive and thus likely not best to put into fixtures. Along the way a few bugs were found, due to refactors that did not have test coverage such as the new package_json_reader abstraction. These permutations should cover all the existing 24 variants in the original file and much more (at time of writing it hits 1300 permutations). I have put enough info in the debuglog('test') to recreate the permutation if you are trying to track down what broke, but it isn't ideal.

@nodejs-github-bot nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. worker Issues and PRs related to Worker support. labels Jul 17, 2020
if (!tmpdirURL.pathname.endsWith('/')) {
tmpdirURL.pathname += '/';
function drainQueue() {
if (spawned < 100 !== true) {

Choose a reason for hiding this comment

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

Is this really clearer than if (spawned >= 100)?

Copy link
Member Author

Choose a reason for hiding this comment

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

don't really agree, but i'm fine changing it

debug(`stderr: ${Buffer.concat(stderr)}`);
throw e;
}
drainQueue();

Choose a reason for hiding this comment

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

This should be inside a finally, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

no, we want to stop spawning things if we have an error

}
throw new Error('unknown format ' + extension);
}
for (const permutation of permutations({

Choose a reason for hiding this comment

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

❤️

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@bmeck
Copy link
Member Author

bmeck commented Jul 30, 2020

CI seems to be unhappy with this change, local testing cannot repro. Wondering if this is related to the large number of child processes spawned, I do see some timeouts but other ones simply exit early with a non-zero code, a few are showing problems with the integrity and i also wonder if the number of files written is a problem.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@bmeck
Copy link
Member Author

bmeck commented Jul 31, 2020

windows failing CI fix is in place, tests are still taking too long and timing out sometimes for CI, unclear on how to fix that.

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-bot
Copy link
Collaborator

@bmeck bmeck force-pushed the policy-fixtures branch from 7d02d07 to 928bdad Compare August 5, 2020 15:15
@bmeck
Copy link
Member Author

bmeck commented Aug 5, 2020

Landed in b04f2b6

@bmeck bmeck closed this Aug 5, 2020
bmeck added a commit that referenced this pull request Aug 5, 2020
PR-URL: #34404
Reviewed-By: James M Snell <jasnell@gmail.com>
codebytere pushed a commit that referenced this pull request Aug 6, 2020
PR-URL: #34404
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Aug 10, 2020
codebytere pushed a commit that referenced this pull request Aug 11, 2020
PR-URL: #34404
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
PR-URL: #34404
Reviewed-By: James M Snell <jasnell@gmail.com>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
PR-URL: #34404
Reviewed-By: James M Snell <jasnell@gmail.com>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
esm Issues and PRs related to the ECMAScript Modules implementation. worker Issues and PRs related to Worker support.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants