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

Allow blessing if test only failed GENERATE #4760

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

samsrabin
Copy link
Contributor

Sometimes a test might fail only in the GENERATE step; for example, if the user doesn't have write permissions in the baseline directory. This PR makes it so that bless_test_results does try to bless things in such cases.

Test suite:
Test baseline:
Test namelist changes:
Test status: [bit for bit, roundoff, climate changing]

Fixes: None

User interface changes?: No

Update gh-pages html (Y/N)?: No

Copy link
Contributor

@jedwards4b jedwards4b left a comment

Choose a reason for hiding this comment

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

I would like to have a review from one of the e3sm developers to make sure that this doesn't adversely affect them.

@jedwards4b jedwards4b requested a review from jasonb5 March 6, 2025 18:41
@samsrabin
Copy link
Contributor Author

The one failing unit test is test_is_bless_needed_overall_fail. Seems like its name and assertions should be changed to reflect that a test failing only in GENERATE should pass blessing. Then another test should be added to check for overall failure due to something other than just GENERATE.

@jasonb5
Copy link
Collaborator

jasonb5 commented Mar 8, 2025

@jgfouca What do you think about this functionality for E3SM?

Copy link
Contributor

@jgfouca jgfouca left a comment

Choose a reason for hiding this comment

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

I think this change is fine for E3SM.

@jedwards4b
Copy link
Contributor

@samsrabin does the unit-testing need to be fixed?

@jasonb5
Copy link
Collaborator

jasonb5 commented Mar 10, 2025

Just need testing to pass and I'll review.

@samsrabin
Copy link
Contributor Author

I'm having trouble fixing the unit testing because I can't figure out where the SMS.f19_g16.A test is being set up. Could one of y'all point me to that?

@jasonb5
Copy link
Collaborator

jasonb5 commented Mar 11, 2025

@samsrabin The test is setup by mocking TestStatus object which is passed to is_hist_bless_needed.

I would update ts.get_status.side_effect = ["PASS", "FAIL"] + ["PASS"] * (len(ALL_PHASES) - 1) to ts.get_status.side_effect = ["PASS",]*len(ALL_PHASES)+1 so the function will still test the original intended behavior.

Then can copy it and update it with ts.get_status.side_effect = ["PASS",]*10 + ["FAIL",] + ["PASS",]*5. This should test when the GENERATE_PHASE is marked as FAIL.

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.

4 participants