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

Add unit tests for the benchmark repo #147

Merged
merged 14 commits into from
Feb 27, 2024
Merged

Conversation

berroar
Copy link
Contributor

@berroar berroar commented Feb 8, 2024

What is the context of this PR?

Adds unit tests for our scripts in our benchmark repo, including a mock output directory. The benchmark repo was initially created without any tests, so it is difficult to gain 100% coverage as a first pass.

We could look at amending some of the logic in future, for example, extracting some of the code from main, which I have done as part of this PR in visualise_results.py to enable better testing. There are some methods that still have no coverage, for example the google_cloud_storage get_files method, as although it is possible to mock the GCS client is difficult to add meaningful tests.

Adds the tests to our actions workflow on PR and enforces coverage at 80%.

Adds isort to benchmark and runs as part of make format.

How to review

Run the unit tests with make test. Check the tests added to make sure they make sense, can any other tests be added?

Check the benchmark is still working.

@berroar berroar force-pushed the add-unit-tests-for-scripts branch from 8e2c2d1 to 173da4d Compare February 12, 2024 12:50
@berroar berroar marked this pull request as ready for review February 12, 2024 13:38
@berroar
Copy link
Contributor Author

berroar commented Feb 12, 2024

Copy link
Contributor

@rmccar rmccar left a comment

Choose a reason for hiding this comment

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

Other than this looks good

scripts/visualise_results.py Outdated Show resolved Hide resolved
Copy link
Contributor

@VirajP1002 VirajP1002 left a comment

Choose a reason for hiding this comment

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

Had a look at the tests and the other changes made and they makes sense to me!

Pipfile Outdated Show resolved Hide resolved
Copy link
Contributor

@liamtoozer liamtoozer left a comment

Choose a reason for hiding this comment

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

Tested locally and works great for me 👍

Pipfile Outdated Show resolved Hide resolved
@berroar berroar merged commit b0cc892 into main Feb 27, 2024
3 checks passed
@berroar berroar deleted the add-unit-tests-for-scripts branch February 27, 2024 10:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants