-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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: initial Azure Pipelines support #7556
Conversation
Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need the corporate CLA signed. If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks! |
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks! |
@kaylangan thanks! @rickhanlonii, do you know who's got the correct permissions? @JoelMarcey perhaps? |
The failing tests seems to mostly be a case of not being able to replace @kaylangan what's the easiest way to debug? I've got access to a windows box through a virtualbox image, and those tests pass there (at least the few I tested now). I'd guess the issue is somehow with this: https://github.com/facebook/jest/blob/7b7fd013508de475e2151e50847373713fc97c7e/e2e/Utils.js#L73-L90 No real need for us to roll our own here either, should be possible to find some module which can do this for us. E.g. npm uses this: https://www.npmjs.com/package/tacks If not that, maybe |
I just gave Azure Pipelines access to the Jest repo. |
@SimenB Sorry for the delayed reply. It looks like the issue with the Windows tests was that TEMP uses the short path name (e.g. |
Thanks!
Not sure I get this. We just use Also, what's the next step now that the app has access to the repo? Is it possible to log in to Azure Pipelines using GH? |
The reason the tests failed were partly due to an inconsistency on the Azure Pipelines end (which we're working on). On our Windows machines, I'd have to do more digging but it appears that either in
Unfortunately, we don't yet have log in with GitHub. But we're working on it! @JoelMarcey when you gave access to the Azure Pipelines app, did you also create an Azure Pipelines organization? If you already have an Azure Pipelines org, you can merge this PR, then in the Azure Pipelines org click "New Pipeline" and it will detect the .azure-pipelines.yml file. Otherwise, you'll want to merge the PR then go through the flow that follows giving the Azure Pipelines app access to the repo (https://github.com/apps/azure-pipelines). |
@SimenB - some, if not all, of the tests were failing because of how Jest was reporting the test paths. When Jest is run from the short/8.3 form of a directory on Windows, "cwd" will stay the short/8.3 name, but "rootDir" (and other paths in the config) get expanded to their long form. When Jest then calculates the relative path for a test (for reporting), the "from" argument is in the short/8.3 form, but the "to" argument is in the normal/expanded form. This results in Jest reporting a path that is technically correct, but not expected ;) This problem isn't unique to Azure Pipelines. You can see it on Windows by running Jest from the short and normal forms of the same directory: Running Jest from normal/expanded directory:
( Running Jest from the short/8.3 form of the same directory:
(the reported path is technically correct, but different from above) I think the right solution requires changes in Jest itself --- either changes to how paths like rootDir are (or are not) normalized, or how relative paths are being calculated for reporting. We "fixed" the problem in Azure Pipelines by setting a variable that causes TEMP to point to a normal/expanded path. Alternatively, we could have changed Make sense? |
Yeah, that makes sense, thanks! I was able to reproduce in a windows image I've got. Will take a look 🙂 |
@willsmythe I opened up #7598 which calls Do you know of any way to normalize the short form to the long form on windows that does not use |
@kaylangan @willsmythe Merged that PR, mind rebasing and seeing if things work without the workaround you added? Also, there's currently a linting error (see https://circleci.com/gh/facebook/jest/42898), a quick |
45a3e15
to
ea7935f
Compare
@SimenB - thanks for the quick fix. We removed the VSTS_OVERWRITE_TEMP workaround, re-ran the build, but still hit some path-related problems on Windows (much fewer than before). The failing tests were findRelatedFiles.test.js, jestRequireActual.test.js, and jestRequireMock.test.js (see build 369 for more details). The root cause for these was code in jest-cli ( In general, it seems like you might need (or need to enforce) a common way to get the current working and temp directories. This would avoid sprinkling There was also a new test failure on macOS that surfaced after rebasing. It was in To answer your previous question, I'm not aware of an alternate way to get the normal/expanded path for a short directory on Windows. |
See latest build results in Azure Pipelines: There is one intermittent test failure on Linux ("can press "u" to update snapshots"). It appears an extra line is occasionally getting picked up in stdout for this test, which causes a mismatch with the snapshot. We will investigate ... |
We moved
@rubennorte thoughts on that? Thank you so much for sticking with me through this btw. Very much appreciated! 🎉 |
8e96cf5
to
dcafcfd
Compare
Looks like everything is green, that's awesome 😀 I'll see if I can ping some FB peeps to do the necessary configuration so we can land this |
@SimenB - What changes do you need? Feel free to ping me if you have trouble getting anything done. |
As a side note, there's still some tests that are skipped on Windows (search for |
Thanks @Daniel15! See this from @kaylangan:
Also, I'm not sure if FB wants an organization, or if Jest should have its own?
Yeah, for sure. I had one passthrough in May (#6310), might be worth having another one |
Ergh, I guess that means we can merge this, so it's detected correctly |
It's probably fine to have a Jest org. The |
Ah, I can do that, cool! Yeah, Did that, however I got an error when trying to confirm. |
Hmm, we may need an admin of the repo to do it. I don't have admin access to this repo, unfortunately. @cpojer Do you know who has admin access to this repo? |
@JoelMarcey has (IIRC, @cpojer does not). |
@SimenB / @Daniel15 - we (Microsoft) can help you get the Azure Pipelines (aka Azure DevOps) organization name you want. In general, we recommend that your Azure DevOps org name matches your GitHub org name (and the project names within this org match your GitHub repo names). If you want to go this route, let us know and we can help. In the meantime, feel free to continue down the path of setting up a "jestjs" organization (it can be renamed later). |
Jest should be approved -- it was before, I just approved a second time. Hopefully that doesn't mess anything up. |
@JoelMarcey I think someone with admin access to the repo needs to actually configure it on the Azure Pipelines end, as it has to add a webhook to the repo. |
I'll send an invite to the org to your fb email. If I understand things correctly, go to https://dev.azure.com/jestjs/jest/_build, hit "new" and follow the prompts |
@SimenB Could you please send me an invite to the org? I usually use daaniel@live.com.au as my Windows Live account, but you could use my FB account danlo@fb.com. Joel can give me temporary admin access to the Jest org to get everything configured. |
Done! The |
Looks like I could add it and it's running successfully: https://dev.azure.com/jestjs/jest/_build/results?buildId=1 |
Yeah, seems like it works! Need a badge for the readme, and give it a week or something then turn off appveyor? |
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
As discussed in #7211, this PR is to add initial support for CI on Azure Pipelines across Mac, Windows, and Linux. Currently, Travis doesn't run master on Windows so there's a test gap.
This PR adds the azure-pipelines.yaml file to run test-ci-partial on the 3 OS's with Node 10. This PR can also be updated to run multiple versions of Node if that's desired. Some of the tests were updated as well.
The tests on Windows are currently failing. One appears to be path separator related and another fs.watch related (which can be finicky on Windows).
You can see the output of the fork @willsmythe set up. He also set up test reporting:
If this change looks good, here's how to proceed to set up Azure Pipelines:
I'll update the CHANGELOG.md once the PR is created.
Test plan
@willsmythe did most of the leg work here and can comment more on the testing he did.