Skip to content

fix: Add tests to cicd checks #2

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.

Sign up for GitHub

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

Open
cweiss95 wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix: Add tests to cicd checks #2

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,4 @@ cicd:
@echo "Running linting and formatting checks..."
@${MAKE} lint
@${MAKE} format

@rwb-aussrc rwb-aussrc May 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@${MAKE} test

@rwb-aussrc rwb-aussrc May 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

@rwb-aussrc rwb-aussrc May 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I.e. have each of these as a separate job in a .github/workfows/CI.yaml file.

@ree13d ree13d May 28, 2026 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@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?

@rwb-aussrc rwb-aussrc May 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@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?

@rwb-aussrc rwb-aussrc May 28, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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).