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.

Sign up for GitHub

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

Jump to bottom

Compclasses cleanup #1226

Merged
merged 5 commits into from
Mar 10, 2017
Merged

Compclasses cleanup #1226

merged 5 commits into from
Mar 10, 2017

Conversation

jedwards4b
Copy link
Contributor

@jedwards4b jedwards4b commented Mar 9, 2017 •
edited
Loading

Get rid of hard coded comp_class settings and use get_values instead.
Punting on NCR test - it's broken, was broken before this PR will open a ticket.

Test suite: scripts_regression_tests.py, hand tests of PET and PEM tests. PEM.f19_g16.F1850.yellowstone_intel
PET.f19_g16.F1850.yellowstone_intel
Test baseline:
Test namelist changes:
Test status: bit for bit

Fixes #1170

User interface changes?:

Code review: gold2718

jedwards4b
jedwards4b commented Mar 9, 2017
View reviewed changes
scripts/lib/CIME/test_scheduler.py
@@ -218,13 +218,15 @@ def __init__(self, test_names, test_data=None,
else:
self._update_test_status(test, phase, TEST_PEND_STATUS)
self._update_test_status(test, phase, status)
logger.info("Using existing test directory %s"%(self._get_test_dir(test)))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unrelated change to improve output from create_test

Copy link

Choose a reason for hiding this comment

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

Since this log output is inside the self._tests loop, wouldn't you end up with identical 'Using existing test directory' log entries if there is more than one test? I suggest either adding the test name to the log output (see below) or move the log outside the for loop.
"Using existing test directory %s for "%(self._get_test_dir(test), test)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The output from _get_test_dir includes the test name.

@gold2718
Copy link

Why are the PET and PEM tests not tested in scripts_regression_tests.py (I know, not really part of this PR review but I'm curious)?

gold2718
gold2718 requested changes Mar 10, 2017
View reviewed changes
Copy link

@gold2718 gold2718 left a comment

Choose a reason for hiding this comment

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

Please document the actual PET and PEM tests performed.

scripts/lib/CIME/test_scheduler.py
@@ -218,13 +218,15 @@ def __init__(self, test_names, test_data=None,
else:
self._update_test_status(test, phase, TEST_PEND_STATUS)
self._update_test_status(test, phase, status)
logger.info("Using existing test directory %s"%(self._get_test_dir(test)))
Copy link

Choose a reason for hiding this comment

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

Since this log output is inside the self._tests loop, wouldn't you end up with identical 'Using existing test directory' log entries if there is more than one test? I suggest either adding the test name to the log output (see below) or move the log outside the for loop.
"Using existing test directory %s for "%(self._get_test_dir(test), test)

scripts/lib/CIME/test_scheduler.py
else:
# None of the test directories should already exist.
for test in self._tests:
expect(not os.path.exists(self._get_test_dir(test)),
"Cannot create new case in directory '%s', it already exists."
" Pick a different test-id" % self._get_test_dir(test))
logger.info("Created test in directory %s"%self._get_test_dir(test))
logger.info("Creating test directory %s"%(self._get_test_dir(test)))

Copy link

Choose a reason for hiding this comment

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

As above, since you moved the statement inside the loop, please add the test name to the log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Try before you comment - the output from _get_test_dir includes the test name.

Copy link

Choose a reason for hiding this comment

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

Oy, yes the test name is buried in the directory name, however, that's not what the user typed. It was just a suggestion to aid users who may not be used to parsing the test directory names.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This change was in response to a user request that just seeing the test name here was not enough and they wanted the full path to the test. I think that the test name is output elsewhere.

@jedwards4b
Copy link
Contributor Author

Regarding the PET and PEM tests - they aren't in cime standalone tests because none of the cime standalone code includes threading and so they wouldn't do anything.

@jedwards4b
Copy link
Contributor Author

I have updated the documentation to reflect the PET and PEM tests that I ran.

@gold2718 gold2718 self-requested a review March 10, 2017 20:17
@jedwards4b jedwards4b merged commit ab579b3 into ESMCI:master Mar 10, 2017
@jedwards4b jedwards4b deleted the compclasses_cleanup branch March 10, 2017 20:44
jgfouca pushed a commit that referenced this pull request Jun 2, 2017
@susburrows
Merge branch 'singhbalwinder/atm/tests-for-4p2' (PR #1226)
f4963b0 
This PR adds SMS_D_Ln5 and ERP_Ld5_P8x4 tests for FC5AV1C-04P2 to the
acme testing suites.

These tests are using ne4 grid resolution.

[BFB] - Bit-For-Bit

* singhbalwinder/atm/tests-for-4p2:
  Adds tests for FC5AV1C-04P2 compset
jgfouca pushed a commit that referenced this pull request Feb 23, 2018
@susburrows
Merge branch 'singhbalwinder/atm/tests-for-4p2' (PR #1226)
ea2c1cd 
This PR adds SMS_D_Ln5 and ERP_Ld5_P8x4 tests for FC5AV1C-04P2 to the
acme testing suites.

These tests are using ne4 grid resolution.

[BFB] - Bit-For-Bit

* singhbalwinder/atm/tests-for-4p2:
  Adds tests for FC5AV1C-04P2 compset
jgfouca pushed a commit that referenced this pull request Mar 13, 2018
@susburrows
Merge branch 'singhbalwinder/atm/tests-for-4p2' (PR #1226)
5b75ba4 
This PR adds SMS_D_Ln5 and ERP_Ld5_P8x4 tests for FC5AV1C-04P2 to the
acme testing suites.

These tests are using ne4 grid resolution.

[BFB] - Bit-For-Bit

* singhbalwinder/atm/tests-for-4p2:
  Adds tests for FC5AV1C-04P2 compset
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gold2718 gold2718 gold2718 approved these changes

Assignees

@gold2718 gold2718

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@jedwards4b @gold2718