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

Windows CI: Avoid downgrading mingw in CI #2596

Merged
merged 5 commits into from
Oct 31, 2023
Merged

Conversation

pramodk
Copy link
Member

@pramodk pramodk commented Oct 29, 2023

- This reverts commit ff55f81  (#2546)
- See #2585 : check if newer image has solved issues seen in #2585
@azure-pipelines
Copy link

✔️ e335287 -> Azure artifacts URL

@azure-pipelines
Copy link

✔️ 8183b09 -> Azure artifacts URL

@azure-pipelines
Copy link

✔️ d665dfe -> Azure artifacts URL

@pramodk
Copy link
Member Author

pramodk commented Oct 29, 2023

@ramcdougal / @adamjhn : Until #2585 is resolved, could you help us to disable running rxd tests?

I did following change in this PR:

- C:\Python311\python -c "import neuron; neuron.test(); neuron.test_rxd(); quit()" || set "errorfound=y"
+ C:\Python38\python -c "import neuron; neuron.test(); quit()" || set "errorfound=y"

but seems like still the tests under /rxdtests runs?

We have an issue on Azure runner while running rxd tests. So instead of disabling rxd on Windows (as part of the build), I would like to just disable running rxd tests. This way, the installer still has rxd enabled.

@codecov
Copy link

codecov bot commented Oct 29, 2023

Codecov Report

Merging #2596 (5f839e4) into master (ca5730e) will increase coverage by 0.00%.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master    #2596   +/-   ##
=======================================
  Coverage   66.07%   66.08%           
=======================================
  Files         559      559           
  Lines      108909   108912    +3     
=======================================
+ Hits        71962    71973   +11     
+ Misses      36947    36939    -8     

see 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@bbpbuildbot

This comment has been minimized.

@ramcdougal
Copy link
Member

I'm generally opposed to disabling tests to get CI to pass. Many of the commits over the last few weeks have been rxd-related, so it's not like this is an area where things aren't changing.

@pramodk
Copy link
Member Author

pramodk commented Oct 30, 2023

I'm generally opposed to disabling tests to get CI to pass. Many of the commits over the last few weeks

Completely agree! I am also not comfortable with just disabling those tests!

Bit of extra info:

  • the installer with Rxd works fine on windows. Just that the GitHub windows runner has strange setup that has issue to run rxd tests
  • atm, we are a bit blocked now and the Windows CI is not running at all. With the current situation, we don't know if non-rxd tests pass.
  • So until Windows CIs failing (again) #2585 is investigated and fixed, if we can just avoid running rxd tests when we check Windows installer then that would be partially helpful. (we are not talking about disabling rxd)

@ramcdougal
Copy link
Member

Broadly the pytest code to be run is defined in https://github.com/neuronsimulator/nrn/blob/master/test/CMakeLists.txt

I'm not sure how you'd only disable it for Windows though.

The other thing that should be fixed: it looks like if rx3d is not enabled then no rxd tests run. 3D being disabled (which I still think is an option that should be removed) should only disable 3d tests, not all rxd tests. In fact, being sure that rxd works without 3d enabled is important.

@sonarcloud
Copy link

sonarcloud bot commented Oct 30, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@azure-pipelines
Copy link

✔️ 5f839e4 -> Azure artifacts URL

@pramodk
Copy link
Member Author

pramodk commented Oct 30, 2023

Broadly the pytest code to be run is defined in https://github.com/neuronsimulator/nrn/blob/master/test/CMakeLists.txt
I'm not sure how you'd only disable it for Windows though.

@ramcdougal : as you might remember, the tests configured in CMake are not executed on Windows. (Due to MingW vs Windows environment differences)

As you can see in the diff of this PR, we are removing call to neuron.test_rxd(); and python share\lib\python\neuron\rxdtests\run_all.py.

In the Test Installer section of Windows setup, you can see what else is currently tested.

I guess this we have to do until find out a way to reproduce/debug & fix the issue.

Copy link
Member

@nrnhines nrnhines left a comment

Choose a reason for hiding this comment

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

It's good that things work again with the "latest" mingw.
@ramcdougal What is your opinion about restoring rxd tests in a subsequent pull request?

@ramcdougal ramcdougal merged commit 53264be into master Oct 31, 2023
35 checks passed
@ramcdougal ramcdougal deleted the pramodk/windows-ci branch October 31, 2023 18:36
JCGoran added a commit that referenced this pull request Feb 8, 2024
Backport of workaround for #2596
JCGoran added a commit that referenced this pull request Feb 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants