-
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: add yarn to lookup.json #905
Conversation
@targos is there any way to see the error details? And had it succeeded? |
There are many failures. You can see the details for each Node.js version with the following links: |
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #905 +/- ##
=======================================
Coverage 96.44% 96.44%
=======================================
Files 28 28
Lines 2139 2139
=======================================
Hits 2063 2063
Misses 76 76 ☔ View full report in Codecov by Sentry. |
Is it ready for another CI round? |
Not yet, we'll also need to skip all big-endian platforms as that's currently not supported. |
is there any way we can know the test machine use big endian or little endian? @targos |
Not sure. @richardlau probably knows better. |
Jest has the same requirement so you can copy its Line 268 in 908ebe2
|
Not directly in the CITGM lookup table, but in the Node.js CI "aix" and "s390x" are big endian.
FWIW of those only the first two ( |
@targos I think we are ready for another CI round |
They all fail with the same error:
|
I can reproduce:
|
Seems 4666750 (#905 (comment)) needs to be reverted, I was assuming it would get the version from the git repo and not the registry. |
Result on my mac after reverting to
|
I don't know why the output is cut. Rerunning with a wider terminal... |
Most (all?) of the fails seem to be around timeouts - perhaps it'd be reasonable to just ignore tests that timeout? 🤔 ~10 tests skipped over almost 1200 is already a good improvement over the existing. |
@arcanis reasonable. Let me turn on the |
@targos do you mind giving it another CI round? |
|
07e7fd9
to
a6d1b3e
Compare
mark |
Does |
@targos hi, it's ready for another CI round. BTW, is it ok to print the I tried to do that (edvardchen@73245e5) but it broke the test case proc.stdout.once('data', () => {
proc.kill('SIGINT');
}); |
This comment was marked as outdated.
This comment was marked as outdated.
Argh. Looks like we're getting the "passes locally but fails in Jenkins CI" thing. Anybody have some time to investigate? I'm not likely to dig too deep today..... EDIT: @arcanis correclty points out the FURTHER EDIT: There's also a heap exhaustion issue in one of the results. YET ANOTHER EDIT: And @arcanis points out a legit failure too. I'm going to run against 20.x instead of the Node.js main branch to see if it clears up other things or not.... |
Running against 20.x: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3289/ |
I've created a new Yarn canary release so that yarnpkg/berry#5763 is included. |
Thanks! I've changed this PR so that it does that. Let's see how that goes.... CI: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3290/ |
f2c52e0
to
b13394e
Compare
Let's see if the unit tests pass anywhere. I think this skips the acceptance tests. I guess we'll see. https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3291/ |
Unit tests passed everywhere except AIX and Windows. My inclination is to skip AIX and Windows and land unit tests only. Adding integration/acceptance tests can be a follow-up PR. |
CI with Node.js 20.x: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3292/ |
I think "skip" is ignored with Node.js 20.x CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/3294/ (will 404 until job starts) |
@Trott You can run |
No yarn failures with this configuration, but we're only running the unit tests and not integration/acceptance tests. I'm inclined to land this and push a release so we have at least some yarn coverage in CITGM. If people want to add additional tests for yarn in subsequent PRs, then great! But even if not, this is better than the current situation. |
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.
Lgtm too; I'll look at enabling some of the most important integration tests in a subsequent PR.
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'm inclined to land this and push a release so we have at least some yarn coverage in CITGM
We technically already do as Jest uses Yarn but I agree that landing this and iteratively adding more tests is better than not running any of our tests.
Checklist
npm test
passes