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

Update restart files of application tests for p4est 2.3.6 #1181

Merged
merged 14 commits into from
Jul 1, 2024
Prev Previous commit
Next Next commit
Use yaml ifs
acdaigneault committed Jun 29, 2024
commit 739152b40c19e68f18cf4baf03581fd2641d1d9d
50 changes: 24 additions & 26 deletions .github/workflows/main_release.yml
Original file line number Diff line number Diff line change
@@ -63,34 +63,32 @@ jobs:
make -j${{ env.COMPILE_JOBS }}
# These tests require a single core each so we will run them in parallel
# Only run tests on deal.ii master version
# Restart files are not compatible with deal.ii v9.5.0
- name: Run Lethe tests (Release-deal.ii:${{ matrix.dealii_version }})
if: ${{ matrix.dealii_version == 'master'}}
run: |
# Only run tests on deal.ii master version
# Restart files are not compatible with deal.ii v9.5.0
if [[ ${{ matrix.dealii_version }} = "master" ]]; then
#Allow OMPI to run as root
export OMPI_ALLOW_RUN_AS_ROOT=1
export OMPI_ALLOW_RUN_AS_ROOT_CONFIRM=1
cd build-release
# Print the tests to be executed
ctest -N --exclude-regex ${{ env.MULTI_CORE_TESTS_REGEX }}
# Run in parallel
ctest --output-on-failure -j${{ env.COMPILE_JOBS }} --exclude-regex ${{ env.MULTI_CORE_TESTS_REGEX }}
fi
#Allow OMPI to run as root
export OMPI_ALLOW_RUN_AS_ROOT=1
export OMPI_ALLOW_RUN_AS_ROOT_CONFIRM=1
cd build-release
# Print the tests to be executed
ctest -N --exclude-regex ${{ env.MULTI_CORE_TESTS_REGEX }}
# Run in parallel
ctest --output-on-failure -j${{ env.COMPILE_JOBS }} --exclude-regex ${{ env.MULTI_CORE_TESTS_REGEX }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add an error when matrix.dealii_version != 'master' for clarity.
Something like:

Suggested change
if: ${{ matrix.dealii_version == 'master'}}
run: ...
else:
error:
message: "Version of deal.ii for tests is not the same as their master image."
code: 1
details: "Restart files are not compatible with deal.ii v9.5.0"

Copy link
Contributor

Choose a reason for hiding this comment

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

That's a cool suggestion.... as long as that does not throw an error I am down for that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Is there a documentation of the else statement and error? I don't find it in the GitHub action documentation.
Also, giving an error won't failed the check?

Copy link
Contributor

Choose a reason for hiding this comment

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

If it gives an error it would indeed fail the check.
To keep the PR finalized because it's a critical one, let's postpone this check to a next PR. We can open an issue ot keep track of this idea, but to be honest, right now just writing a comment is all good for the time being. Throwing an error would break the CI and so code: 1 would most likely give an error.
Let's postpone that for the future

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

So I don't change anything at the moment?

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope, let's not change anything. Let's merge the PR as-is when you are ready with the current feature set. Then if we want to change other stuff, let's make small PRs for them

# These tests require two cores each so we will run them sequencially
# Only run tests on deal.ii master version
# Restart files are not compatible with deal.ii v9.5.0
- name: Run multi-core Lethe tests (Release-deal.ii:${{ matrix.dealii_version }})
if: ${{ matrix.dealii_version == 'master'}}
run: |
# Only run tests on deal.ii master version
# Restart files are not compatible with deal.ii v9.5.0
if [[ ${{ matrix.dealii_version }} = "master" ]]; then
#Allow OMPI to run as root
export OMPI_ALLOW_RUN_AS_ROOT=1
export OMPI_ALLOW_RUN_AS_ROOT_CONFIRM=1
cd build-release
# Print the tests to be executed
ctest -N --tests-regex ${{ env.MULTI_CORE_TESTS_REGEX }}
# Run sequencially
ctest --output-on-failure --tests-regex ${{ env.MULTI_CORE_TESTS_REGEX }}
fi
#Allow OMPI to run as root
export OMPI_ALLOW_RUN_AS_ROOT=1
export OMPI_ALLOW_RUN_AS_ROOT_CONFIRM=1
cd build-release
# Print the tests to be executed
ctest -N --tests-regex ${{ env.MULTI_CORE_TESTS_REGEX }}
# Run sequencially
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add the same else error as the previous comment here.

ctest --output-on-failure --tests-regex ${{ env.MULTI_CORE_TESTS_REGEX }}