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 SingleJob and Results classes #138

Conversation

dormrod
Copy link
Contributor

@dormrod dormrod commented Aug 27, 2024

Description

Add tests for the SingleJob and Results classes. In addition include small refactoring.

Refactorings

There are small refactorings here. Add a StrEnum called JobStatus to explicitly define the different job status types. This is now used throughout the code. Note this is backwards compatible as string comparisons should all still work as previously.

In addition add type hints to the Job and Result classes

Bugfix

One small bug fix flagged by tests - Job.load did not actually return the loaded job, now it does.

Tests

Otherwise add tests for the SingleJob and Results classes. To facilitate this, add a DummySingleJob which runs a simple shell command, possibly with a wait. This allows testing of the job file generation and dependencies.

@dormrod dormrod marked this pull request as draft August 27, 2024 07:54
@dormrod dormrod force-pushed the DavidOrmrodMorley/SCMSUITE-8674.single_job_and_results_unit_tests branch from d00f57d to dd015e0 Compare August 27, 2024 08:00
@dormrod dormrod marked this pull request as ready for review August 27, 2024 08:03
Copy link
Contributor

@mhellstr mhellstr left a comment

Choose a reason for hiding this comment

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

DummySingleJob seems instructive enough to put it somewhere with all the other job classes and not hidden away in a test, so that users can import it / so that one could create some instructive example in the docs.

There is also an option to add an arbitrary wait period before the job runs to simulate a longer-running calculation.
"""

def __init__(self, inp=None, cmd=None, wait=0.0, **kwargs):
Copy link
Contributor

Choose a reason for hiding this comment

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

add type hints

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added

@dormrod
Copy link
Contributor Author

dormrod commented Sep 4, 2024

DummySingleJob seems instructive enough to put it somewhere with all the other job classes and not hidden away in a test, so that users can import it / so that one could create some instructive example in the docs.

Interesting idea. I don't think I would want to put it with the production jobs code, but perhaps it is worth creating a test_utils sub package (or similar) which contains a reusable dummy job.

For now I think I will leave as-is in the tests, until there are enough useful components that it would be worth breaking them out and including them in the package, or actually used by an example. The class itself is very thin anyway at the moment.

@dormrod dormrod force-pushed the DavidOrmrodMorley/SCMSUITE-8674.single_job_and_results_unit_tests branch from dd015e0 to 53d7d20 Compare September 16, 2024 16:14
@dormrod dormrod requested a review from mhellstr September 17, 2024 09:22
Copy link
Contributor

@mhellstr mhellstr left a comment

Choose a reason for hiding this comment

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

Looks good. Would be nice to at some point have a pedgogical example that shows the awk_file, awk_output, grep_file, etc. similar to what is being done in the test here but more easily available to users.

@dormrod dormrod merged commit 5f3915a into trunk Sep 18, 2024
14 checks passed
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.

2 participants