fix: Add tests to cicd checks#2
Conversation
rwb-aussrc
left a comment
There was a problem hiding this comment.
Hi Charles,
Thank you for putting together these changes. I have some comments about the general design of this and I think I'd request some modifications:
- Instead of one global
cimake target, instead use thejobfunction within the github workflows to call each make target. - Remove the
format --fixmake target from any CI-related job, because I don't think it actually does what we think it does; instead, see if it can produce an error code if the code needs formatting. - Ensure these changes are reflected in the workflows CI yaml files.
| @echo "Running linting and formatting checks..." | ||
| @${MAKE} lint | ||
| @${MAKE} format | ||
| @${MAKE} test |
There was a problem hiding this comment.
This approach to CI/CD would not be my preferred; ideally, we would separate each of these so that if one fails, it doesn't affect the output of the others. It also means that if we need to re-run one of the tests, we don't re-run all of them.
There was a problem hiding this comment.
I.e. have each of these as a separate job in a .github/workfows/CI.yaml file.
There was a problem hiding this comment.
@rwb-aussrc while I do think they should be seperate if for no other reason that they can then run in parallel. If we have to rerun tests odds are the code has had to be changed/committed so they need to all rerun anyway no?
There was a problem hiding this comment.
@ree13d I'm thinking more in the situation that there is a VM timeout or something intermittent.
...no I have not been burned by this exact thing many times in the past, what are you talking about?
| @@ -24,3 +24,4 @@ cicd: | |||
| @echo "Running linting and formatting checks..." | |||
| @${MAKE} lint | |||
| @${MAKE} format | |||
There was a problem hiding this comment.
Has anyone confirmed that:
format:
ruff check --fix src tests
Works as a CI/CD option? As far as I can tell, this will:
- fix the code it finds in the CI image
- do nothing else.
There's no command to commit those changes, so the actual content of the branch won't change. I don't think format works as a CI policy except to highlight (through an error code) that someone should fix that locally and then commit it.
| @echo "Running linting and formatting checks..." | ||
| @${MAKE} lint | ||
| @${MAKE} format | ||
| @${MAKE} test |
There was a problem hiding this comment.
These changes are not reflected or used in the .github/workflows section - we should try and aim to add these changes to the workflows so that someone else doesn't add a call to the workflows in the future to compensate for their absence, only to find that this particular make target exists (although see above comment about why I don't think we should have one ci make target).
|
Thanks for your comments @rwb-aussrc. I'm very happy for you to take over this work whilst I'm away, if you have time. Although finding appropriate time may be the challenging part. Otherwise I can take a look when I'm back from my leave at the end of June. I like your approach of making the various "stages" separate jobs in the gibhub workflows yaml. That sounds very reasonable to me and aligns with "stages" in GitLab. |
This MR aims to call the make tests target in the CICD make target so that tests are also run in the CICD make target.