-
Notifications
You must be signed in to change notification settings - Fork 37
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
Change test cases to run like suites #350
Conversation
Example outputRunning the
|
TestingI ran the ocean I also ran several test cases and steps. Step output looked the same as usual. Test case output looked like the example above. |
@mark-petersen, @matthewhoffman and @altheaden, this is ready for your comments and review. This work is needed in order for @altheaden to make headway so it is not super urgent but would be helpful to have in the next 2-3 weeks at the latest. @mark-petersen and @matthewhoffman, hopefully, it's easy to review. What I would be looking for (assuming you're okay with the concept) is to set up the branch and run a test case on its own to make sure 1) it works and 2) you're happy with the output. |
@xylar & @altheaden , I'm starting to look at this now and hope to have completed my testing tomorrow. After perusing this initially, the only thing I noticed that is worth discussing so far is the naming convention - I like the concept of merging the two run functions, but I'm wondering if the name for that combined function deserves further thought. Using |
I'm fine with |
37b04a7
to
3391317
Compare
I rebuilt the docs and ran a local suite, test case and step to make sure they all work with the new |
@xylar , I ran a suite before and after this change. I looked at the output for a single test that was part of the suite for both versions, and it looked identical. Is that expected? (I would guess yes, but just confirming I'm not missing something.) |
@xylar, I ran a test case manually before and after the change. The image below shows the diff in the information printed to the terminal before (left) and after (right). It looks like a convenient side effect for running test cases like suites is that when a test case is run, you get a little more information printed: the name of the test at the beginning and a summary of PASS/FAIL and runtimes. That's a nice addition. The only thing we've lost is the |
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.
@xylar , I'm approving this with 1 minor typo to fix in the docs and pending your judgment of the two general comments I made. I can see how this change will be really advantageous in the long run.
3391317
to
7add695
Compare
@matthewhoffman, thank you for the review. I addressed your comments, including adding back text like:
|
This reduces redundancy and gives better output (such as timing) when running at test case on its own.
It is now `run_tests` rather than `run_suite`.
7add695
to
bc4f9ef
Compare
Thanks, @xylar ! And thanks for adding back that extra line of output. |
@mark-petersen , I tested this pretty thoroughly, so it's probably sufficient for you to just ensure a test case runs fine for you. |
That all looks good to me, and I agree about the |
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.
Tested on grizzly with gnu and nightly suite, also single test case. Everything works great, thanks.
Thanks, @mark-petersen! |
This merge makes several changes that are useful in preparing for task parallelism.
The most noticeable is that it gets rid of
run_test_case()
and instead usesrun_tests()
[renamed fromrun_suite()
] to run both suites and single test cases. This has at least 2 potential advantages: it reduces redundant code and it gives additional useful output when running a test case on its own (notably the timing of the test case).The other change will not be noticed by users. I moved
compass/run.py
intocompass/run/serial.py
in anticipation of having a secondcompass/run/???.py
module that will handle task-parallel execution.The merge also updates the documentation to account for the new locations of the functions and the new behavior.