Ensure tests skipped due to associated compile failure get added to test_changes.list; group rt log failure messages by failure reason

[gspetro-NOAA]

Commit Queue Requirements:

• This PR addresses a relevant WM issue (if not, create an issue).
• All subcomponent pull requests (if any) have been reviewed by their code managers. N/A
• Run the full Intel+GNU RT suite (compared to current baselines), preferably on Ursa (Derecho or Hercules are acceptable alternatives). Exceptions: documentation-only PRs, CI-only PRs, etc.
  • Commit log file w/full results from RT suite run (if applicable).
  • Verify that test_changes.list indicates which tests, if any, are changed by this PR. Commit test_changes.list, even if it is empty.
• Transparency in the use of generative AI is required by NOAA policy. Was GenAI used in this work? Just asked Gemini some questions!
  • [Generative AI tool NOAA Gemini] was used to assist with developing this code. The code has been reviewed, edited, and validated by NWS staff.
• Fill out all sections of this template.

---

Description:

When a compile fails, the tests that are skipped are not added to test_changes.list; this PR fixes that. Additionally, this PR renames the -b option to -s (for subset) because it can be used to run any subset of RTs formatted like test_changes.list (e.g., "test_name compiler"). The -b option implies that it can only be used to create a new baseline for a subset of tests in combination with the -c option.

In addition to running the full RT suite, I tested this PR by:

• Altering UFSATM/fv3/atmos_cubed_sphere/model/fv_dynamics.F90 with a line ("THIS_IS_AN_INTENTIONAL_COMPILATION_ERROR !!!") to cause a compile fail.
• Running nohup ./rt.sh -a epic -e -s run.list & with the following run.list file:

  cpld_warmstart_c12_9deg intel
  hafs_global_storm_following_1nest_atm intel
  conus13km_2threads gnu
  control_diag_debug gnu
  control_restart_p8 intel
  datm_cdeps_control_cfsr intelllvm
  cpld_control_gefs intel
  cpld_restart_gefs intel
  cpld_dcp_gefs intel
  

Only the datm test passed, which makes sense given the lack of active atm component. All of the other tests, plus control_p8 intel (needed for control_restart_p8 intel) appear in test_changes.list.

UPDATES:
This PR also reworks rt.sh to group failed/skipped tests by failure/skip reason. For example, see:
RegressionTests_ursa_3313a.log.

Commit Message:

* UFSWM - Bugfix to ensure tests skipped due to associated compile failure get added to test_changes.list
* UFSWM - Group RT log messages for failed/skipped tests by failure/skip reason

Priority:

• Critical Bugfix: Reason
• High: Reason
• Normal

Git Tracking

UFSWM:

• Closes test_changes.list does not report tests that failed bc their compile failed #3018

Sub component Pull Requests:

• None

UFSWM Blocking Dependencies:

• Blocked by #
• None

Documentation:

• Documentation update required.
  • Relevant updates are included with this PR.
  • A WM issue has been opened to track the need for a documentation update; a person responsible for submitting the update has been assigned to the issue (link issue).
• Documentation update NOT required.
  • Explanation:

---

Changes

Regression Test Changes (Please commit test_changes.list):

• PR Adds New Tests/Baselines.
• PR Updates/Changes Baselines.
• No Baseline Changes.

Input data Changes:

• None.
• PR adds input data.
• PR changes existing input data.

Library Changes/Upgrades:

• Required
  • Library names w/versions:
  • Git Stack Issue (JCSDA/spack-stack#)
• No Updates

---

Testing Log:

• RDHPCS
  • Orion
  • Hercules
  • GaeaC6
  • Derecho
  • Ursa
• WCOSS2
  • Dogwood/Cactus
  • Acorn
• CI
• opnReqTest (complete task if unnecessary)

Testing Remarks:

[DeniseWorthen]

@gspetro-NOAA Maybe I'm misunderstanding, but I consider test_changes.list the list of tests whose baselines change. If the PR code doesn't even compile for some test(s), then we don't want to associate 'failed compile' with 'baseline change'.

The PR with a failed compile simply needs fixing before any testing should proceed. I'd like to see the RT log certainly spell this out much more boldly; it is easy to miss. But hiding a failed compile in the test_changes.list doesn't really solve the problem of someone making a PR, expecting some tests to fail anyway and missing that some tests actually failed because they didn't compile.

[gspetro-NOAA]

@gspetro-NOAA Maybe I'm misunderstanding, but I consider test_changes.list the list of tests whose baselines change. If the PR code doesn't even compile for some test(s), then we don't want to associate 'failed compile' with 'baseline change'.

The PR with a failed compile simply needs fixing before any testing should proceed. I'd like to see the RT log certainly spell this out much more boldly; it is easy to miss. But hiding a failed compile in the test_changes.list doesn't really solve the problem of someone making a PR, expecting some tests to fail anyway and missing that some tests actually failed because they didn't compile.

@DeniseWorthen I agree that the committed test_changes.list file should only contain tests that failed due to a baseline comparison error because they are expected to change baselines. Currently, however, a test may end up listed in test_changes.list because it failed for other non-PR-related reasons during testing, such as a disk quota issue or test time out. Realistically, the developer always needs to check the logs to see why things failed and if they need fixing. This is true whether the test failed in compile or failed during testing with, e.g., FAILED: RUN DID NOT COMPLETE. And whether a rerun or a fix-and-rerun is required, the developer (or CM) should ideally be able to rerun all tests that didn't pass, whether they were skipped due to compile failure or failed mid-test. To my knowledge, we don't have any functionality that does this, so we end up looking in rt.conf and copy-pasting test names and compilers from under the failed compiles. If there's already a better way, I'd love to know! This was particularly problematic with Derecho testing (although now things are okay there other than the super long queue times).

I think things are getting confused because the -b option is somewhat misleadingly named to imply that it only applies to baseline generation. Technically, running with the -b option (and no -c) will work to run a subset of tests, and the file it takes in could be named anything. So -b is not exclusively used for baseline generation of a subset of tests. Similarly, the file produced by an RT run will be called test_changes.list regardless of whether test baselines change. We could rename it failed_tests.list, but I like the idea of keeping the current name because when used locally, the name doesn't matter much, and when pushed to the PR, it does (or should) reflect the tests whose baselines are expected to change.

[DeniseWorthen]

I went back and looked at the original PR that led to the issue creation; the PR in that case was made by one of our most careful contributors and even then the "noise" of a compile failure didn't break through.

I pasted the end of the log file from that PR into an AI prompt "What messaging would you suggest to clearly announce loudly and boldly that there were failures in addition to just baseline changes."

The suggestion was to cut through the noise by delineating the message types:

While I don't disagree in principle that adding a compile fail to a test_changes.list could be useful (as long as it was not committed), I think the root of the problem that led to the original issue formation is the messaging, not the test_changes.list

[gspetro-NOAA]

The messaging is definitely big part of the challenge. We've run into the issue in other PRs, where there were a lot of expected baseline changes and one/a few other little failures snuck through. PR #3130 comes to mind. As a result, I put in some CI that parses out which tests failed for what reasons, but it's combined with the warnings/remarks check and doesn't produce an actual failure (because sometimes we expect tests to fail). It could be improved, but it would probably be more useful to have the failure messaging in the logs so no one has to go looking for it. I will see about updating the logs to print failures category by category as the AI suggested... 🤔
With this potential addition, are you ok with the changes that are already in?

[DeniseWorthen]

Realistically, the developer always needs to check the logs to see why things failed and if they need fixing. This is true whether the test failed in compile or failed during testing with, e.g., FAILED: RUN DID NOT COMPLETE. And whether a rerun or a fix-and-rerun is required, the developer (or CM) should ideally be able to rerun all tests that didn't pass, whether they were skipped due to compile failure or failed mid-test. To my knowledge, we don't have any functionality that does this, so we end up looking in rt.conf and copy-pasting test names and compilers from under the failed compiles. If there's already a better way, I'd love to know! This was particularly problematic with Derecho testing (although now things are okay there other than the super long queue times).

I do agree that manually editing the rt.conf for failures is not ideal, and if that is the primary purpose here, I think that is OK as far as it goes. I would love to see better log messaging though.

[gspetro-NOAA]

All tests passed successfully. Skipping Derecho due to ongoing maintenance and Acorn due to ongoing unavailability.