-
Notifications
You must be signed in to change notification settings - Fork 207
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 option to define test types in query_testlists #1930
Conversation
@@ -354,7 +357,7 @@ NODEFAIL Tests restart upon detected node failure. Generates fake failu | |||
<DOUT_S>FALSE</DOUT_S> | |||
</test> | |||
|
|||
<test NAME="TESTBUILDFAIL"> | |||
<test NAME="TESTBUILDFAIL" INFRASTRUCTURE_TEST="TRUE"> |
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 don't know if this was the right way to do this. I wanted some way to exclude these infrastructure-only tests, because otherwise they clutter the output. I'm open to suggestions of a better way to denote this.
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.
Seems like a decent approach to me. The other way would be check if the test is a subclass of FakeTest.
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.
Nearly all of the tests except "homme" work with stand alone cime. "homme" in particular only works with the acme version of the atmosphere component. Can we make that clear in this output?
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.
See inline.
@@ -354,7 +357,7 @@ NODEFAIL Tests restart upon detected node failure. Generates fake failu | |||
<DOUT_S>FALSE</DOUT_S> | |||
</test> | |||
|
|||
<test NAME="TESTBUILDFAIL"> | |||
<test NAME="TESTBUILDFAIL" INFRASTRUCTURE_TEST="TRUE"> |
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.
Seems like a decent approach to me. The other way would be check if the test is a subclass of FakeTest.
expect(not(args.count and args.list_type), | ||
"Cannot specify both --count and --list arguments.") | ||
|
||
if args.count: |
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 might be cleaner to achieve with ArgParse's mutually exclusive group (see case.build).
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.
That's a useful-looking feature. But I'm struggling with how to do that here. I see how I can replace this with a mutually exclusive group:
expect(not(args.count and args.list_type),
"Cannot specify both --count and --list arguments.")
But I can't see how to express the rest of the logic with that mechanism: Although it's not allowed to combine either --show-options
or --define-testtypes
with either --list
or --count
, it is allowed to combine --show-options
with --define-testtypes
. Can you see how to express this with a mutually exclusive group?
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.
OK, if it's not obvious then don't worry about it.
Would the right thing to do here be to move the homme test into ACME's CAM's cime_config? In the meantime, I'll just edit the DESC string for that test to add, "Only works with the ACME version of the atmosphere component." |
I just rediscovered #1685 after submitting this PR. I still feel like it's useful to have this information available via query_testlists, but maybe we want it in create_test, too? @jedwards4b @jgfouca @fischer-ncar if it's okay with you, I'll leave the extension to create_test up to @fischer-ncar as part of resolving #1685 . You should be able to reuse much of what I did here. |
As far as I could tell, we didn't have any way to query the different
test types and their meaning (ERS, ERP, etc.). This PR adds this
capability to query_testlists.
I'm not sure if query_testlists is exactly the right place for this, but
I couldn't think of anywhere better. I thought about query_config, but I
didn't want to clutter that tool with this option that is more for
developers than users. So I have implemented this as an optional header
that can be output with query_testlists. I'm open to suggestions for a
better user interface.
The output looks like this:
(and then a lot more tests following that, since this lists out all the
tests defined in cime).
Test suite: scripts_regression_tests on cheyenne
Test baseline: N/A
Test namelist changes: none
Test status: bit for bit
Fixes #1929
User interface changes?: adds --define-testtypes option to query_testlists
Update gh-pages html (Y/N)?: N
Code review: