-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
[CI] Run tests on Windows #20260
Comments
A while back @matthargett suggested that to fix the CircleCI "Windows absence" we could integrate AppVeyor CI |
Recently babel add Appveyor too. |
I agree that AppVeyor CI would be a good alternative and it looks like from their homepage that Facebook is already one of their great customers. As starting point, maybe we can introduce some basic Windows tests like:
With this we should be able to catch issues like #20015. |
I have created a pr #20281, hope it helps. |
Yeah I think that's a "good enough" start, and then we can ask Windows dev to jump in and add more tests as the platform becomes more widely used. |
Okay, the basic setup is okay, and we can wait for facebook/metro#181 to be fixed. |
So what test should on windows platform run ? Aside from basic android build. I also add some Personally I want to add metro debug server bundle works as expected. |
@hramos Can you check the appveyor config or the badge url. Looks like README.md now has not show any build in appveyor. |
The windows issue (#20353) still exists on master branch now. So we do need a test case to test the request to I will see what I can do after my two prs got merged. |
Currently the android build works on appveyor, but we have some jest test failure.
Most of those related to
cc @rafeca |
Just an FYI: I just enabled facebook/react-native on Appveyor. Builds should be running now. |
I have a fix for the |
@jeanlauliac Hopefully we can fix the windows ci via your pr :) |
Summary: @public These tests are using a mock memory FS to start with, so there is no reason at all they should depend on the host OS or filesystem details. This changeset fixes that so that we fully mock the `fs` and `path` modules dependending on the mock platform (not the host platform). I also added an example of how we can test both platforms (regardless of the host platform) in `findPackageClassName`. Follow up changeset will be to do the same for all the other affected tests. Related to #20260. Reviewed By: mjesun Differential Revision: D9771024 fbshipit-source-id: b368b43e8e54292d33b6183eec9a9ea69f2e6e76
Please try past e327f88, hopefully that adresses issues with the |
@jeanlauliac Good job. Test case fail number on windows from 54 to 7. |
I have create a pr to fix remaining failed case: #21203.
cc @jeanlauliac Can you help on this ? |
Yeah that's because |
9c242c8 should fix the issue. Let me know. |
@jeanlauliac It looks like cause more problem, now it have three fails, plz check https://ci.appveyor.com/project/facebook/react-native/branch/master/job/9s95x3eb1f937qrk. |
Summary: @public These tests are using a mock memory FS to start with, so there is no reason at all they should depend on the host OS or filesystem details. This changeset fixes that so that we fully mock the `fs` and `path` modules dependending on the mock platform (not the host platform). I also added an example of how we can test both platforms (regardless of the host platform) in `findPackageClassName`. Follow up changeset will be to do the same for all the other affected tests. Related to facebook/react-native#20260. Reviewed By: mjesun Differential Revision: D9771024 fbshipit-source-id: b368b43e8e54292d33b6183eec9a9ea69f2e6e76
The errors on the report are not related to the JS tests are they? It looks like build errors. |
Nope, it's test case. |
We're not looking at the same thing then. In the link you sent, here's the only error I can see:
What am I missing? 🙂 |
Here's the error report from the above link:
A few of these appear to be issues reading from the filesystem on Windows. For example:
|
@jeanlauliac ping |
The changeset partly fixes things only, I think :) for the rest I'd have to grab a windows VM and try out. As for the |
@jeanlauliac Glad to hear, I hope we can fix this soon. Maybe @rafeca can help you with the windows part, he fix some metro issue on windows machine. |
Since this issue was created, Microsoft released this: Which may be an option if appveyor is being too troublesome. That |
@empyrical Windows 10 ? I have the issue on my win10 too. |
Appveyor is still red -- what do you folks think about removing Appveyor.yml from the master branch until this is fixed? It makes it hard to see when Circle CI tests have broken when looking at the list of recent commits. I hope Appveyor does let you run tests on a branch that includes Appveyor.yml, similar to how Circle CI works, as that would help verify the status of those tests. |
I've used AppVeyor without issue on my React Native fork, not sure about other branches on the same repo though. But if someone wants to try and fix this last remaining test on their fork it would work fine. |
@empyrical Can you share your success link in your react native fork ? |
Oh, sorry, I didn't get the test working on my fork. What I mean is, if someone wants to make an attempt at fixing the failing |
Summary: @public These tests are using a mock memory FS to start with, so there is no reason at all they should depend on the host OS or filesystem details. This changeset fixes that so that we fully mock the `fs` and `path` modules dependending on the mock platform (not the host platform). I also added an example of how we can test both platforms (regardless of the host platform) in `findPackageClassName`. Follow up changeset will be to do the same for all the other affected tests. Related to facebook#20260. Reviewed By: mjesun Differential Revision: D9771024 fbshipit-source-id: b368b43e8e54292d33b6183eec9a9ea69f2e6e76
Summary: @public Partly fixes facebook#20260 Reviewed By: rafeca Differential Revision: D10302150 fbshipit-source-id: 2d9a63b263c9e27c22989c447ce884934f1e4430
For Discussion
Our suite of tests does not currently run on Windows machines. This results in a blind spot that has allowed issues affecting Windows users to creep into the code base (facebook/metro#181).
Circle CI does not support running jobs on Windows at the moment. I'd like to open a discussion and get feedback from the community on how to proceed.
The text was updated successfully, but these errors were encountered: