-
Notifications
You must be signed in to change notification settings - Fork 148
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
lookup: declare bankruptcy on flaky modules #958
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #958 +/- ##
==========================================
+ Coverage 96.27% 96.46% +0.18%
==========================================
Files 28 28
Lines 2149 2149
==========================================
+ Hits 2069 2073 +4
+ Misses 80 76 -4 ☔ View full report in Codecov by Sentry. |
"minimist": { | ||
"npm": true, | ||
"maintainers": "substack" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you link me to the failures here? afaik minimist should be passing (altho "maintainers" needs to be updated)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can just skip Windows for minimist unless and until someone gets around to looking into why it's not working on our Windows infrastructure?
https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3213/nodes=win-vs2019/testReport/junit/(root)/citgm/minimist_v1_2_8/
https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3212/nodes=win-vs2019/testReport/junit/(root)/citgm/minimist_v1_2_8/
https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3211/nodes=win-vs2019/testReport/junit/(root)/citgm/minimist_v1_2_8/
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, please do!
"tape": { | ||
"head": true, | ||
"prefix": "v", | ||
"maintainers": "substack" | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i think only one of the windows builds is failing, so instead of removing it, could that be skipped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@GeoffreyBooth noted that 20.6.0 had problems and that I should probably use 20.5.x as the baseline for failed tests (or "failed both 20.6.0 and 20.5.x") so I'll do that. |
"failed" isn't the same as "flaky" tho, at least for is-core-module and resolve. |
Remove all modules that failed the latest 20.x release run. We need to be able to trust that a failed CITGM run is something that needs to be investigated.
Can we ping module authors here, so they are aware of this action? We should merge it anyway |
I would love for us to be able to trim down the list a bit more in order to help with build times but we can work on it in a follow-up PR. |
Whoops, I pushed to main by accident and now it marked this as merged. 🙃 Sorry. I'll force push it back to where it was in a moment and open a new PR. 😞 |
This is awesome :) |
Remove all modules that failed the latest 20.x release run. We need to be able to trust that a failed CITGM run is something that needs to be investigated.
Checklist
npm test
passeshere