-
Notifications
You must be signed in to change notification settings - Fork 5
Better modularisation #162
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
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.
Nice idea! And I indeed have a few thoughts :)
- As you said, there is duplication and I think using a reusable workflow would be an overkill in this case. I think it would be better to use a single workflow with steps representing these different modules. Then these steps would be run only if the
if
condition would be satisfied. Something like
steps:
- name: 'Test core'
if: core_files in changed_files
run: python3 -m pytest $CORE_TESTS
- name: 'Test Laplacian smoothing'
if: laplac_smoothing_files in changed_files
run: python3 -m pytest $LAPL_TESTS
and so on... I haven't tested the exact syntax for this if condition, but something like if: ${{ github.event.pull_request.changed_files && 'movement/laplacian_smoothing.py' in github.event.pull_request.changed_files }}
should do it perhaps. But it should be expanded to also trigger if the workflow is triggered by the schedule event, or the push to main branch event - which are straightforward. Edit: alternatively could use https://github.com/marketplace/actions/changed-files again which offers this fine-grained control.
With this PR, the code coverage check is only run as a weekly job.
I guess you don't measure coverage in individual workflows because it leads to seemingly lower coverage? We can pass the --include
flag to specify what files should be included in the coverage report. Something like python3 -m coverage run --include="movement/math.py,movement/tangling.py" -m pytest test/test_math.py test/test_tangling.py
for TEST in ${TESTS}; do | ||
python3 -m pytest -v -k "parallel[1] or not parallel" "${TEST}" | ||
done |
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.
Could you please clarify why you're looping over individual tests, instead of just running them all at once with python3 -m pytest -v -k "parallel[1] or not parallel" ${TESTS}
? Same elsewhere.
I mentioned the other day that it'd be good to better modularise things in the mesh adaptation code bases. Movement is already nicely modularised and the three mesh movement implementations don't interact with one another.
This PR separates the test suite out to accelerate development of code related to just one of the implementations. The demos are still run with any code changes, although we could introduce subdirectories for the demos to separate them, too. Doing so would require modifying
test_demos.py
to enable running the demos for just one subdirectory. With this PR, the code coverage check is only run as a weekly job.Requesting comment from @ddundo because I expect you might have some thoughts on all this. There's quite a bit of duplication in the workflows so this can probably be avoided with reusable workflows.