-
Notifications
You must be signed in to change notification settings - Fork 150
Added tests on Windows #364
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
base: main
Are you sure you want to change the base?
Conversation
178d561 to
d4abdfc
Compare
| @@ -0,0 +1,50 @@ | |||
| name: test-windows | |||
| run-name: Tests Windows | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please re-use the test.yaml instead of creating dedicated workflow files. If you want to organize them in individual files, try a design like this.
The main thing is that we should be gating different stages of the CI, e.g. always check a pre-commit first before doing long tests.
| - name: Set conda environment | ||
| uses: mamba-org/setup-micromamba@main | ||
| with: | ||
| environment-name: myenv | ||
| environment-file: environment-dev.yml | ||
| init-shell: bash | ||
| cache-downloads: true | ||
| create-args: | | ||
| ninja |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please no conda/mamba. If you wish to check it make it a distinct test set. Instead for windows please use vcpkg as the package manager.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The use of micromamba was to easily get the dependencies, I don't have the need to test it specifically and can use whatever you prefer. Out of curiosity, why are you opposed to using micromamba?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, why are you opposed to using micromamba?
Mostly that it is the wrong tool in CI. If the purpose is to test the conda ecosystem then it can make sense for that, but then it would be better to have the project packaged there. But for testing OS, using the primary package manager of that OS would be preferred, particularly important being the usage of the native compilers, in this case being msvc.
The use of micromamba was to easily get the dependencies
We are using FetchContent in order to have a package manager independent source of dependencies. Testing the package managers is also quite important since it can have specific patches applied, but in that case only the packaged packages must be used.
| - name: Configure using cmake | ||
| run: cmake -Bbuild -DCMAKE_BUILD_TYPE:STING=Release -DCMAKE_INSTALL_PREFIX=$CONDA_PREFIX -G ninja | ||
|
|
||
| - name: Build | ||
| working-directory: build | ||
| run: ninja | ||
|
|
||
| - name: Run tests | ||
| working-directory: build | ||
| run: ctest -R ^test$ --output-on-failure |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Modern approach is to use CMake presets, and I believe this one already has it.
| strategy: | ||
| fail-fast: true | ||
| matrix: | ||
| json_version: [ develop, v3.12.0, v3.8.0 ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be more explicit if you are testing FetchContent dependencies or package manager ones.
No description provided.