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

chore: add jest to lookup #728

Merged
merged 7 commits into from Mar 7, 2021
Merged

chore: add jest to lookup #728

merged 7 commits into from Mar 7, 2021

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented May 10, 2019

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 passes
  • tests are included
  • documentation is changed or added
  • contribution guidelines followed
    here

@SimenB SimenB mentioned this pull request May 10, 2019
@targos
Copy link
Member

targos commented May 10, 2019

@codecov-io
Copy link

codecov-io commented May 10, 2019

Codecov Report

Merging #728 (bfaba7a) into main (d0185d9) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #728   +/-   ##
=======================================
  Coverage   96.12%   96.12%           
=======================================
  Files          31       31           
  Lines         929      930    +1     
=======================================
+ Hits          893      894    +1     
  Misses         36       36           
Impacted Files Coverage Δ
lib/grab-project.js 92.00% <100.00%> (+0.16%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d0185d9...bfaba7a. Read the comment docs.

@targos
Copy link
Member

targos commented May 10, 2019

The TypeScript build took almost 10 minutes in CI. Is that expected?

18:08:23 verbose: jest yarn-install:  | Building TypeScript definition files
18:08:23 verbose: jest yarn-install:  | Building..................................................................
18:17:06 verbose: jest yarn-install:  | DONE

@SimenB
Copy link
Member Author

SimenB commented May 10, 2019

Woah, not that much. More like 3.

We should skip it regardless, but weird it's that high

@BridgeAR
Copy link
Member

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.

@SimenB
Copy link
Member Author

SimenB commented May 11, 2019

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
jestjs/jest#8397

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?

@targos
Copy link
Member

targos commented May 11, 2019

I'm running locally with Node 10

@BridgeAR
Copy link
Member

@SimenB sounds good to me! I suggest to keep the other module in that case. Otherwise some breaking changes might not be detected.

@targos
Copy link
Member

targos commented May 11, 2019

First problem: it depends on Mercurial being installed on the host.

@targos
Copy link
Member

targos commented May 11, 2019

One error remains after installing hg:

verbose: jest npm-test:      |   ● gets changed files for hg
verbose:                     |                                                                                                                                                                                                                                              

verbose:                     | Command failed: hg status -amnu --rev min((!public() & ::.)+.)^ /tmp/1c37a264-9b80-4620-9839-67106cadda8b/npm_config_tmp/jest-changed-files-test-dir /tmp/1c37a264-9b80-4620-9839-67106cadda8b/npm_config_tmp/jest-changed-files-test-dir/nes
ted-dir /
verbose:                     | abort: empty revision range                                                                                                                                                                                                                  

verbose:                     |                                                                                                                                                                                                                                                
verbose:                     |       at makeError (node_modules/execa/index.js:174:9)                                                                                                                                                                             
verbose: jest npm-test:      | .                   
verbose: jest npm-test:      | .                   
verbose: jest npm-test:      | .                   
verbose: jest npm-test:      | .                   
verbose: jest npm-test:      | .                   
verbose: jest npm-test:      |   ● gets changed files for hg                                                         
verbose:                     |                                                                                       
verbose:                     | expect(received).toMatch(expected)                                                    
verbose:                     |                                                                                       
verbose:                     | Expected pattern: /PASS __tests__(\/|\\)file2.test.js/                                
verbose:                     | Received string:  "                                                                   
verbose:                     |                                                                                       
verbose:                     |   ● Test suite failed to run                                                          
verbose:                     |                                                                                       
verbose:                     |     abort: empty revision range                                                       
verbose:                     |                                                                                       
verbose:                     | "
verbose:                     |
verbose:                     |       299 | 
verbose:                     |       300 |   ({stdout, stderr} = runJest(DIR, ['-o', '--changedFilesWithAncestor']));
verbose:                     |     > 301 |   expect(stderr).toMatch(/PASS __tests__(\/|\\)file2.test.js/);
verbose:                     |           |                  ^
verbose:                     |       302 |   expect(stderr).toMatch(/PASS __tests__(\/|\\)file3.test.js/);
verbose:                     |       303 | });
verbose:                     |       304 | 
verbose:                     |
verbose:                     |       at Object.toMatch (e2e/__tests__/onlyChanged.test.ts:301:18)
verbose:                     |       at asyncGeneratorStep (e2e/__tests__/onlyChanged.test.ts:13:103)
verbose:                     |       at _next (e2e/__tests__/onlyChanged.test.ts:15:194)
verbose:                     |       at e2e/__tests__/onlyChanged.test.ts:15:364
verbose:                     |       at Object.<anonymous> (e2e/__tests__/onlyChanged.test.ts:15:97)

@targos targos added the WIP Work in progress label May 18, 2019
@Zirro Zirro mentioned this pull request May 29, 2019
@SimenB
Copy link
Member Author

SimenB commented Aug 22, 2019

@targos I noticed some of the mercurial tests should be skipped on CI - does CITGM set the CI env variable?

https://github.com/facebook/jest/blob/522ed17dcebf988450b93ddb8bf748249c75de82/e2e/__tests__/onlyChanged.test.ts#L255-L262

When it comes to build time, if we could make the install step be node ./scripts/remove-postinstall && yarn && yarn build, it'll drop the TS compile which should speed things up. That's what we do for CI already: https://github.com/facebook/jest/blob/522ed17dcebf988450b93ddb8bf748249c75de82/.travis.yml#L14

@targos
Copy link
Member

targos commented Aug 22, 2019

@SimenB I don't know if CITGM sets any env variable but you could add something like envVar: { CI: 'true' } to the lookup entry.

@SimenB
Copy link
Member Author

SimenB commented Aug 22, 2019

Great point! Added that know. I guess we still need to install mercurial, but I think it might pass now

@SimenB
Copy link
Member Author

SimenB commented Oct 29, 2019

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)

@targos
Copy link
Member

targos commented Oct 29, 2019

It looks like we need to exclude PPC and Windows

@targos
Copy link
Member

targos commented Oct 29, 2019

You can add "master": true to the lookup table to use the master branch instead of the latest release

@targos
Copy link
Member

targos commented Oct 29, 2019

And "skip": [">=12"] to skip Node 13 and 14-pre.

@SimenB
Copy link
Member Author

SimenB commented Oct 29, 2019

We run CI on windows, odd it fails here... Added master: true and just landed a fix for node 13, so hopefully it'll be better. Still need to have hg installed, though

@targos
Copy link
Member

targos commented Oct 29, 2019

@SimenB
Copy link
Member Author

SimenB commented Oct 29, 2019

Ah, windows is missing build tools for https://github.com/node-ffi-napi/weak-napi. Any chance of adding those?

EDIT: node-ffi-napi/weak-napi#21

@targos
Copy link
Member

targos commented Oct 29, 2019

What are they?

@targos
Copy link
Member

targos commented Oct 29, 2019

The error I see is c:\program files\git\citgm_tmp\090df5f1-f59b-4a39-be96-f858a9a9abb7\jest\node_modules\weak-napi\src\weakref.cc(2): fatal error C1083: Cannot open include file: 'setimmediate.h': No such file or directory [C:\Program Files\Git\citgm_tmp\090df5f1-f59b-4a39-be96-f858a9a9abb7\jest\node_modules\weak-napi\build\weakref.vcxproj]

@targos
Copy link
Member

targos commented Oct 29, 2019

@addaleax any idea?

@SimenB
Copy link
Member Author

SimenB commented Oct 29, 2019

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

@targos
Copy link
Member

targos commented Oct 29, 2019

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.

@targos
Copy link
Member

targos commented Oct 29, 2019

Still need to have hg installed, though

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?

@targos
Copy link
Member

targos commented Oct 20, 2020

CI: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker-pipeline/129/

Running citgm-all --includeTags jest this time, to test the skip list.

@SimenB
Copy link
Member Author

SimenB commented Oct 20, 2020

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

@targos
Copy link
Member

targos commented Oct 20, 2020

10.x in skip works: https://ci.nodejs.org/job/citgm-smoker-nobuild/984/

"prefix": "v",
"maintainers": ["cpojer", "scotthovestadt", "SimenB", "thymikee", "jeysal"],
"yarn": true,
"scripts": ["build:js", "test-ci-partial"],
Copy link
Member Author

Choose a reason for hiding this comment

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

this ran tests locally, but seems like CI skipped it? very odd...

image

I'll have to try tomorrow

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 idea about this - the tests were ran on mac, but timed out.

@SimenB
Copy link
Member Author

SimenB commented Oct 21, 2020

macOS is incredibly slow, so let's skip it

@SimenB
Copy link
Member Author

SimenB commented Oct 21, 2020

@targos when convenient, yet another try would be appreciated 🙂

@SimenB
Copy link
Member Author

SimenB commented Oct 21, 2020

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

@SimenB
Copy link
Member Author

SimenB commented Oct 24, 2020

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

@targos
Copy link
Member

targos commented Mar 5, 2021

@targos
Copy link
Member

targos commented Mar 5, 2021

It fails on macOS with ENAMETOOLONG. Is that a known issue?
https://github.com/targos/citgm/runs/2038722798?check_suite_focus=true

@SimenB
Copy link
Member Author

SimenB commented Mar 5, 2021

not beyond me mentioning it here 😅 #728 (comment)

We could probably start by landing only for linux at first so it's in?

@SimenB
Copy link
Member Author

SimenB commented Mar 5, 2021

Very nice with a GH action, tho! Easier to find the logs at least for me than jenkins 😀

@SimenB
Copy link
Member Author

SimenB commented Mar 6, 2021

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

@targos
Copy link
Member

targos commented Mar 6, 2021

Okay, could you please rebase the PR and add win32 to the skip list?

@targos
Copy link
Member

targos commented Mar 6, 2021

@SimenB
Copy link
Member Author

SimenB commented Mar 6, 2021

@targos rebased and windows skipped

@targos targos merged commit 9d3cd93 into nodejs:main Mar 7, 2021
@targos
Copy link
Member

targos commented Mar 7, 2021

@SimenB thank you! It was a long journey :)

@SimenB SimenB deleted the jest branch March 7, 2021 11:23
@SimenB
Copy link
Member Author

SimenB commented Mar 7, 2021

🎉🎉

Almost 2 years 🙈

MylesBorins pushed a commit that referenced this pull request Mar 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WIP Work in progress
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Jest to lookup
6 participants