-
Notifications
You must be signed in to change notification settings - Fork 166
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
Enable Windows Arm64 tests in CI #3046
Comments
Cc @pbo-linaro, @joaocgreis, @mhdawson, @sxa Moving the email thread to GitHub issue for visibility and tracking |
Job windows-arm-simple is now set to run daily, to evaluate stability in time. |
@pbo-linaro Last nightly run was failing https://ci.nodejs.org/job/windows-arm-simple/nodes=win10-vs2019-arm64/39/. Could you have a look ? |
After investigation, I can confirm it is flaky tests, randomly failing because of wrong order in output expected. Those tests are concerned:
|
Thanks, @pbo-linaro ! Is it possible to fix the tests to not depend on a particular order? |
I'm trying to contact author of those commits to see if that is normal, and what we can do with it. |
Have you got a link to the relevant commits/PRs for reference - is it the ones that added those tests? And do they have the same problem on win-x64? The bug theme of this year in multiple projects I've been on has been sorting/ordering problems! |
PR:
@MoLow (author). Do you know if something could create trouble with this? On my machine, I can make some failure appear by increasing the charge of machine, so something clearly has problem with async dependencies. |
@sxa I suppose win-x64 CI did not fail with this, else those tests should already been marked as flaky. On our WoA machines, we reproduced that with x64 binaries (instead of arm64), so it's not architecture/code specific. |
|
Thanks @richardlau, I missed that one, but that's exactly the issue. |
Not sure if it's the correct thread to report this, but I'm getting |
@aduh95 It is not built on an arm64 machine, but on this agent: you can probably open another issue, or simply ask on nodejs slack build channel, if you have access to it. |
ARM64 Windows was never enabled by default in the main I'm not sure we can enable it by default for all runs with the current resources we have now. The machines we have might be enough, but I expect many jobs will queue up when CI is under load. Also, if the machines fail, it would be good to have someone around who knows how to disable only ARM64, without disabling the whole Windows job. I suggest we try enabling it, but be ready to disable it quickly if it starts causing issues. We only need to edit the main fanned job and change the default value of RUN_ARM64_TESTS to be set by default. If tests are failing on master, at some point the procedure was to mark them flaky in https://github.com/nodejs/node/blob/main/test/sequential/sequential.status until a fix lands. I tried running CI for v18.11.0 with ARM64 enabled, but it didn't run any of the relevant test-binary configurations. For example for https://ci.nodejs.org/job/node-test-binary-windows-js-suites/17311/: I see RUN_ARM64_TESTS is true in https://ci.nodejs.org/job/node-test-binary-windows-js-suites/17311/parameters/ but it is considered false in https://ci.nodejs.org/job/node-test-binary-windows-js-suites/17311/console . I checked the safeParameters list and it was not there, so I added it. I believe Jenkins needs to be restarted to update that variable, I'll do that over the weekend if it doesn't happen before. Anyway, unless that variable was recently updated, the cause might be anything else. |
That would be very nice to start running them. For flaky tests reported here, I created this PR nodejs/node#45049. Different platforms reported problems, so I assume it was good to mark them flaky for all of them. |
@joaocgreis To clarify things a bit more, neither @nsait-linaro nor me have access to what you describe, so that would be nice if you could change that value to enable tests for this platform. |
Sounds good. Let's start the run and we can keep an eye on the load and stability. We can add more machines/agents if more resources are required. @pbo-linaro I guess we have to wait for your PR nodejs/node#45049 to land before setting |
Thanks to nodejs/node#45049 merged, the windows-arm-simple is now green again! 👍 Can we start enabling official job for this platform (despite current limitation with only two machines available)? Thanks |
Builds have been working fine for the last few days, I think we should enable it and see it in action. @joaocgreis @sxa Could you please help to enable it ? |
Hi - yes I've been tied up with release work elsewhere but I should be able to look at this in the next few days, under the caveat that if there are any problems with it the first course of action will likely be to disable it again immediately to avoid any problems with contributors being able to run PR tests :-) |
Yes sure, we don't want to block anything for contributors! The windows-arm-simple job has been green since we fixed it, so it should be fine. |
Just to give an update from my side, I still don't understand why the test-binary jobs don't invoke any of the ARM configurations, the fix I tried above didn't work. Enabling the checkbox by default will not do anything until we're able to fix this. |
@joaocgreis Is there someone responsible for NodeJS jenkins instance that could help to debug this configuration? |
I'm back from vacation now :-) I had a shot at re-enabling in the CI and it's running at https://ci.nodejs.org/job/node-compile-windows/nodes=win-vs2019-arm64/ albeit a bit slow so is causing some delays (I just re-enabled the cross-compiled jobs that were already in the system to get something running) but that should be resolved when nodejs/node#45432 goes in. We should probably also persue native builds (consistent with what we've been doing in https://ci.nodejs.org/job/windows-arm-simple/) potentially as a separate pipeline for now so should work on getting https://ci.nodejs.org/job/node-test-commit-windows-arm running and use that as our primary build+test pipeline for windows/arm instead of the cross-compilation now. |
Thanks @sxa. Is there anything we can do to help with this? Seems like its jenkins configuration work, for which we don't have access for now. |
Yes, who has the privilege to set it up? |
@sxa @joaocgreis @pbo-linaro @StefanStojanovic Can we have a sync call to figure out the next steps for this? This has been pending for quite some time and I think we could probably find a way forward with a short sync-up. |
There was a bug which hadn't been spotted from when we upgraded the jenkins server to Java 11 that prevented the |
Second step is to try and get this running as a "normal" (non-fanned) combined build+test job like we do on most of the other platforms, which is what node-test-commit-window-arm will do - with the limited amount of machines here, and certainly not the multitude of windows versions that we have for x64 it doesn't make too much sense to use the fanned jobs. It's likely to be trivial stuff to fix given that we've got this all working in the
@joaocgreis @StefanStojanovic I'm guessing we don't have a natural path through the Windows build scripts that be activated for a combined build+test at the moment (e.g. excluding the need for |
@StefanStojanovic and I are testing the Ansible scripts for ARM64 Windows, we're getting close to having VMs in CI. As part of this, Stefan is running the scripts on the nearform machines as well and disabled nssm. Please expect some instability in the two machines for a few days. |
@pbo-linaro We seem to have quite a lot of failed tests in the CI just now. Is that expected? I wouldn't have through they were directly related to the work that @joaocgreis and @StefanStojanovic are doing just now |
Yes, we have been discussing this with @joaocgreis and @StefanStojanovic. The plan is to deactivate the 5 concerned tests, reenable the CI (so no more regressions appear), and then fix those 5 tests. Considering they are already broken in current state, it's not wrong to do it this way. On the opposite, trying to fix anything before CI is enabled would be wrong. |
Just a heads up: support for Windows 11 on Parallels on Apple Silicon Macs is now official: |
Windows 10 and 11 machines occasionally show finish setting up device screen thus preventing Jenkins from starting after reboot. These changes disable that behavior. Refs: nodejs#3046
Windows 10 and 11 machines occasionally show finish setting up device screen thus preventing Jenkins from starting after reboot. These changes disable that behavior. Refs: nodejs#3046 PR-URL: nodejs#3211 Reviewed-By: Richard Lau <rlau@redhat.com>
A quick update: VS2019 is not officially supported for running on ARM64 Windows, so we've had to use VS2022. We now have 6 ARM64 Windows VMs connected to CI. Here is a test run: https://ci.nodejs.org/job/node-test-commit-windows-fanned/54031/ . This run is for nodejs/node#47020 , and once that PR lands we can move forward here and enable tests to run by default. |
Thanks, @joaocgreis. nodejs/node#47020 has been landed. Hope we can enable the CI by default now. |
ARM64 Windows tests are running by default 🎉 Will monitor for issues for a few days, this can then be closed after #3250 lands. |
Excellent! Great work @joaocgreis and @StefanStojanovic! |
Refs: nodejs#45943 Refs: nodejs#47020 Refs: nodejs/build#3046 Refs: nodejs/build#3246 Refs: nodejs/build#3250 Refs: nodejs/build#3211
Refs: nodejs/build#3046 Refs: nodejs/build#2540 Fixes: nodejs#25998
Refs: #47020 Refs: nodejs/build#3046 PR-URL: #47235 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Refs: #47020 Refs: nodejs/build#3046 PR-URL: #47235 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Refs: nodejs/build#3046 Refs: nodejs/build#2540 Fixes: #25998 PR-URL: #47233 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Refs: nodejs/build#3046 Refs: nodejs/build#2540 Fixes: #25998 PR-URL: #47233 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Refs: nodejs/build#3046 Refs: nodejs/build#2540 Fixes: #25998 PR-URL: #47233 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Refs: #47020 Refs: nodejs/build#3046 PR-URL: #47235 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Refs: nodejs/build#3046 Refs: nodejs/build#2540 Fixes: #25998 PR-URL: #47233 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Richard Lau <rlau@redhat.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Refs: #47020 Refs: nodejs/build#3046 PR-URL: #47235 Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Debadree Chatterjee <debadree333@gmail.com>
Node CI had a pipeline for testing Windows Arm64 platform sometimes back but it got disabled as some tests were failing. See #2540 (comment)
Linaro has been looking at reviving the CI job and now we have a new standalone job ( https://ci.nodejs.org/job/windows-arm-simple/ ) that is running with all tests passing.
As discussed during the build meet-up in #2994, Re-enabling the CI again would be crucial to avoid regressions and to officially support the Windows Arm64 platform as requested in #2540
We will need to re-enable the Windows Arm64 test stage to https://ci.nodejs.org/job/node-test-binary-windows-js-suites/
The text was updated successfully, but these errors were encountered: