-
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
chore: add jest to lookup #728
Conversation
Codecov Report
@@ Coverage Diff @@
## main #728 +/- ##
=======================================
Coverage 96.12% 96.12%
=======================================
Files 31 31
Lines 929 930 +1
=======================================
+ Hits 893 894 +1
Misses 36 36
Continue to review full report at Codecov.
|
The TypeScript build took almost 10 minutes in CI. Is that expected?
|
Woah, not that much. More like 3. We should skip it regardless, but weird it's that high |
The tests still seem to fail due to jest dependencies not working properly? I am on my phone, so it would be good if someone else would verify that. |
We have a failure due to https://github.com/TooTallNate/node-weak not compiling on node 12, should probably skip master and 12 until we figure something out. jestjs/jest#8411 Not sure about the optipng-bin thing on windows. Besides that, looks like we need mercurial installed and figure out the enametoolong thing. Ideas on that one? |
I'm running locally with Node 10 |
@SimenB sounds good to me! I suggest to keep the other module in that case. Otherwise some breaking changes might not be detected. |
First problem: it depends on Mercurial being installed on the host. |
One error remains after installing
|
@targos I noticed some of the mercurial tests should be skipped on CI - does CITGM set the When it comes to build time, if we could make the install step be |
@SimenB I don't know if CITGM sets any env variable but you could add something like |
Great point! Added that know. I guess we still need to install mercurial, but I think it might pass now |
We'll need master rather than a published version for node 12, and node 13 has a known error with yarn pnp (open PR, not yet merged: yarnpkg/yarn#7650) |
It looks like we need to exclude PPC and Windows |
You can add |
And |
We run CI on windows, odd it fails here... Added |
Ah, windows is missing build tools for https://github.com/node-ffi-napi/weak-napi. Any chance of adding those? |
What are they? |
The error I see is |
@addaleax any idea? |
it might be node-ffi-napi/weak-napi#21 rather than any missing tools (I just assumed when I saw the complilation error)... It passes on windows in our CI, see e.g. latest: https://dev.azure.com/jestjs/jest/_build/results?buildId=3634 I don't know what differenes between azure pipelines windows and jenkins windows are |
We have a problem, https://ci.nodejs.org/job/citgm-smoker-nobuild/679/nodes=fedora-latest-x64/console is marked as SUCCESS even though the tests failed. |
I'm not sure that being able to run all Jest tests is a compelling reason to install Mercurial on all our CI hosts. @nodejs/build What do you think? |
CI: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/129/ Running |
I see node 15 has been released (congrats!). The PR I linked above which worked in the nightly has not been released, so if Node 15 has been added to CITGM it'll fail. I can make a release tomorrow if needed, or we can just look at master |
|
"prefix": "v", | ||
"maintainers": ["cpojer", "scotthovestadt", "SimenB", "thymikee", "jeysal"], | ||
"yarn": true, | ||
"scripts": ["build:js", "test-ci-partial"], |
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.
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.
no idea about this - the tests were ran on mac, but timed out.
macOS is incredibly slow, so let's skip it |
@targos when convenient, yet another try would be appreciated 🙂 |
Another thing, seems we have a test that failed on windows, but citgm says it passed? https://ci.nodejs.org/job/citgm-smoker-nobuild/nodes=win10-vs2019/986/console |
I actually noticed a Windows failure on our own CI that GH Actions said was successful. Might have to take a look into that... In the meantime, perhaps skipping windows as well and just test on linux? Better than nothing |
Testing with a new GitHub workflow I created: https://github.com/targos/citgm/actions/runs/624042087 |
It fails on macOS with |
not beyond me mentioning it here 😅 #728 (comment) We could probably start by landing only for linux at first so it's in? |
Very nice with a GH action, tho! Easier to find the logs at least for me than jenkins 😀 |
Both Windows and macOS are somewhat flaky on our own CI as well, so it might be a good idea regardless to just go for the Linux version for now |
Okay, could you please rebase the PR and add win32 to the skip list? |
@targos rebased and windows skipped |
@SimenB thank you! It was a long journey :) |
🎉🎉 Almost 2 years 🙈 |
We've had a release, so in theory the color issue should be solved. We've sped up our own CI by skipping the TypeScript build, should probably make the same change here. For now I just thread through a way higher timeout.
I haven't run this locally, I just figured I'd open up the PR before being computerless for the weekend. 🤞
Fixes #684
Checklist
npm test
passeshere