-
Notifications
You must be signed in to change notification settings - Fork 7
Better document testing requirements for Omega PRs #304
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
base: develop
Are you sure you want to change the base?
Better document testing requirements for Omega PRs #304
Conversation
This page describes running the `omega_pr` test suite for each Omega PR.
The updated version provides better links to the documentation and a more thorough list of testing requirements.
|
A test build of the documentation can be found here: |
|
@cbegeman, would you be willing to take a look at this and make sure it addresses the discussion we had 2 or 3 weeks ago about better documenting the requirement to run the |
|
@sbrus89, would you be willing to give this a look or recommend a different reviewer? |
|
Note: I ran into problems with the |
|
@xylar This looks really great! One situation that could be helpful to address is how developers should proceed if they are making changes that affect the namelist/streams (i.e., make changes to mpaso_to_omega.yaml and test-specific yaml files if relevant). |
sbrus89
left a comment
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, this is a really helpful guide. Thanks for putting it together. I just have a few minor comments.
| CTest unit tests | ||
| - Machine(s): <machine>[, <partition>] | ||
| - Compiler(s): <compiler>[, ...] | ||
| - Build type(s): <Debug|Release>[, ...] | ||
| - Result: All tests passed | Failures: <name> (<one-line note>), ... | ||
|
|
||
| Polaris omega_pr regression suite | ||
| - Baseline build (-p): <path/to/develop/build> | ||
| - Baseline workdir (-w): <path/to/baseline_omega_pr> | ||
| - PR build (-p): <path/to/pr/build> | ||
| - PR workdir (-w): <path/to/my_branch_omega_pr> | ||
| - Machine/partition: <machine>[, <partition>] | ||
| - Compiler/build type: <compiler>, <Debug|Release> | ||
| - Result: All tests passed | Diffs/Failures: <brief summary> | ||
| - Logs (if applicable): <path/to/polaris_omega_pr.o$JOBID> |
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.
Can we configure CTest/Polaris to produce this output in a log file, so the developer doesn't have to recreate it by hand?
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.
Not really. It's the output from a bunch of different things.
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.
Sorry for dismissing this yesterday. I've thought about it more and I think we can make each tool produce the output we want here, especially if we slightly alter the formatting.
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.
I looked into this for CTest and I don't see an easy way to get it to produce this information. We could try to get the utility from Polaris to produce it, but that would require capturing the output from CTest itself and parsing it. Not at all impossible but maybe not trivial.
As for the Polaris tests, I don't think Polaris has a way to know whether Omega was build in debug or release mode but we could write out the rest of this info at the end of the omega_pr suite. I can work on that as I have time.
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.
I implemented the remainder of this capability in E3SM-Project/polaris#387. We will need to add documentation here about where to find the file to copy/paste from once that PR has gone into Polaris.
Yes, very good point, @cbegeman. |
|
Do you want to try and add PR testing with github actions? |
@rljacob, that would be a good idea but it's a pretty big project for us to take on right at the moment. I think we're going to have to deal with manual testing for a bit here. |
| - Machine(s): <machine>[, <partition>] | ||
| - Compiler(s): <compiler>[, ...] | ||
| - Build type(s): <Debug|Release>[, ...] | ||
| - Result: All tests passed | Failures: <name> (<one-line note>), ... |
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.
I updated the CTest utility to produce output like this in E3SM-Project/polaris#385.
| For instructions on how to build Omega and run CTest unit tests, see | ||
| {ref}`omega-dev-quick-start-build-test`. | ||
|
|
||
|
|
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.
This section introduces a link to build Omega, but it does not explicitly state that building the develop branch and the test branch of Omega are required steps for the subsequent instructions even though those information is provided in following sections. Adding one or two statements explaining the Omega build requirements and how they relate to the DEVELOP_BUILD and TEST_BRANCH_BUILD paths would be helpful for users trying this for the first time.
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.
@grnydawn, thank you. I have attempted to clarify this in c0fa7a5, and also corrected some incorrect statements about what should be used for the baseline in aa1d5e3 and 3409373. Let me know if this now looks good to you. I rebuilt at:
https://web.lcrc.anl.gov/public/e3sm/diagnostic_output/xasaydavis/test_docs/html/
but it might not stay there for long since I use that URL for test-building documentation for various purposes.
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.
Thanks for the update. It looks good to me. I was able to successfully run the test by following the directions on the page.
|
@sbrus89, could you let me know if you have further comments here? |
grnydawn
left a comment
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.
I successfully ran the test by following the directions on the page using the Intel compiler on Chrysalis.
There were a few failed tests, which I will summarize in a separate comment, but I don't think they block approval of this PR.
|
During testing for this PR review, I encountered the following failed test result. To verify the b4b check, I used the same Omega commit for both the baseline and the test branch, so I expected the b4b tests to pass. Below is the last part of the log from the test branch: and from the baseline branch: I attached the full log file of the test too. |
|
@grnydawn, it looks from the list of tests like you're using the Polaris |
|
@grnydawn, also what machine and which compiler was this on? |
|
And thank you so much for testing things out and reviewing! |
I ran the test using the Intel compiler on Chrysalis. |
Okay, great! We've already tested the new branch with that configuration and it's fixed so I don't think you need to redo your testing. Thanks again for running through the process! |
Sure, I will do that. By the way, what do you mean by the main branch? Was my test configuration not correct? |
The
No, it's our fault on the Polaris side. We didn't notices several breaking changes that happened in Omega over the past couple of months so we're getting |
This merge adds a "Code Testing" page to the documentation that describes running the
omega_prtest suite foreach Omega PR.
It also updates the PR checklist to better describe testing. The updated version provides better links to the documentation
and a more thorough list of testing requirements.
It includes many fixes to typos in the Quick Start.
Checklist