Skip to content

[develop]: Add regression testing - #28

Merged
mkavulich merged 74 commits into
developfrom
feature/regression_tests
Jul 17, 2026
Merged

[develop]: Add regression testing#28
mkavulich merged 74 commits into
developfrom
feature/regression_tests

Conversation

@georgemccabe

@georgemccabe georgemccabe commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

DESCRIPTION OF CHANGES:

Note: This PR contains changes from PR #25 to address some issues running the tests. Until that PR is merged, the differences in this PR only can be viewed from this link:
feature/fix_some_issues...feature/regression_tests

  • Adds files in tests/regression directory
    • run_regression.py - run all end-to-end tests for a given branch/commit, pull request, or the "baseline" (commit/branch that is considered to be the truth data to compare new test results)
    • run_diff.py - Compare output generated by run_regression.py to other output to see if there are any differences using the METplus diff util
    • README.md - instructions and examples to run regression test scripts
    • regression_baseline.py - stores branch (develop) and commit that is considered the baseline to compare new results. If a PR will changes these results and is expected, the value in this file should be updated to the latest commit of the develop branch after the PR has been merged.
    • regression_common.py - variables/functions common to multiple scripts, e.g. the default location of the regression test output and the name of this repo
    • environment.yml - conda environment file to create vx_diff environment used to run the run_diff.py script
  • Updated setup_conda.sh script to take an argument vx_diff which creates/activates the diff environment instead of the vx_workflow environment

An example of the log file created by the diff script can be found on ursa:
/scratch3/BMC/dtc/George.Mccabe/dtc-vx-workflow_testing/diff_WE2E_20260630200405.txt

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

TESTS CONDUCTED:

  • derecho.intel
  • gaeac6.intel
  • hera.gnu
  • hera.intel
  • hercules.intel
  • orion.intel
  • ursa.gnu
  • ursa.intel
  • NOAA Cloud (indicate which platform)
  • Jenkins
  • fundamental test suite
  • comprehensive tests (specify which if a subset was used)

DEPENDENCIES:

DOCUMENTATION:

ISSUE:

CHECKLIST

  • My code follows the style guidelines in the Contributor's Guide
  • I have performed a self-review of my own code using the Code Reviewer's Guide
  • I have commented my code, particularly in hard-to-understand areas
  • My changes need updates to the documentation. I have made corresponding changes to the documentation
  • My changes do not require updates to the documentation (explain).
  • My changes generate no new warnings
  • New and existing tests pass with my changes
  • Any dependent changes have been merged and published

LABELS (optional):

A Code Manager needs to add the following labels to this PR:

  • Work In Progress
  • bug
  • enhancement
  • documentation
  • release
  • high priority
  • run_ci
  • run_we2e_fundamental_tests
  • run_we2e_comprehensive_tests
  • help wanted

CONTRIBUTORS (optional):

georgemccabe and others added 30 commits May 28, 2026 10:29
…is currently written to be run from that directory
…ed up test runs. Clean up logic to report if a test dir is not found in either baseline or new output
 - Replace call to load_modules_wflow.sh with call to setup_conda.sh
 - Since we no longer have module loads, ROCOTO_PATH is provided as a variable in the machine file for each platform
Read ROCOTO_PATH from the machine config file and prefix all calls to
rocotorun and rocotostat with it, consistent with launch_vx_wflow.sh.
Machines without ROCOTO_PATH (jet, noaacloud, linux, macos) fall back
to bare binary names. rocoto_path is stored in the WE2E monitor YAML
and propagated through update_expt_status/compare_rocotostat; added to
all non-task entry skip lists to avoid being treated as a task.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The statuses loop in update_expt_status had 8-space indentation while
the other two skip lists had 12-space indentation, so the replace_all
edit missed it. This caused a TypeError when iterating over the expt
dict since rocoto_path (a string) was not excluded from the task loop.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Resolve SCRIPT_DIR from ${BASH_SOURCE[0]} at the top and anchor all
relative file/directory references (conda_loc, conda/, environment.yml)
to it. Also replaces readlink -f with cd && pwd for macOS compatibility
without requiring GNU coreutils.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

@mkavulich mkavulich left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Looks good, I have just a few comments/suggestions.

Comment thread tests/regression/run_diff.py
Comment thread tests/regression/README.md Outdated

The default behavior is to run the diff utility on all files in the dated subdirectories
under the output directory, because these are assumed to contain the actual MET output.
The `--diff_all_files` argument can be added to run the diff utility on all files in the output directory.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm a bit confused about the behavior of this flag. It seems like the SKIP_KEYWORDS files are still skipped even when this flag is enabled...is that the intended behavior? If so, I think this flag needs a better name, since it implies that all files will be diffed when active.

@mkavulich mkavulich Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It would also be nice if there was a flag to allow you to run a diff where it ignores missing test directories...for example, if you only run a subset of all tests, but still want to check them for success against the baseline.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Regarding the --diff_all_files argument, I agree that it is a little misleading. The default behavior is to run the diff on each YYYYMMDDHH directory under each workflow directory, and the --diff_all_files argument runs the diff on each workflow directory, which will include some of the other files, like input files that are downloaded.

You are correct that the SKIP_KEYWORDS are still used if the flag is set or not. Excluding this list of skip keywords would result in a lot of misleading diffs being flagged in both configurations.

Any suggestions for improving this? Maybe the --diff_all_files argument is not really needed if we are only concerned with comparing the MET output? I added this option with the intention of being able to see if there are differences in the input files that could change at the source unexpectedly, which would explain the differences in the output. Maybe this is not a common occurrence and unnecessary to easily check?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Regarding an option to run a subset of tests:

This is a good idea. I could add an optional --tests argument for the run_regression.py script to specify a subset of tests that could be passed to the run_we2e_tests.py script to tell it to run that subset. Currently it just calls it with --tests all.

A corresponding argument could be added to the run_diff.py script to determine which sub-directories (tests) to run the diff on. This would require pulling out the logic that parses the --tests argument in run_we2e_tests.py so it could be used by the run_diff.py script in the same way. Thoughts?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

With regards to the --diff_all_files option, I think renaming it to --diff_obs or --diff-input or something similar would suffice.

I think utilizing "tests" logic from run_we2e_tests.py sounds great.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@mkavulich, I renamed the --diff_all_files argument to --diff_inputs to run_diff.py and added the --tests argument to run_regression.py to run a subset of tests by passing the value of the argument directly to run_we2e_tests.py.

I am working on updating the run_diff.py to take a --tests argument to run the diff logic on a subset of tests in another branch. These changes are a little more involved to share the functions to gather/check the e2e test names and isolate the vx_workflow and vx_diff dependencies, so I suggest that those changes be added to a separate PR. Does that sound good to you?

georgemccabe and others added 3 commits July 15, 2026 11:35
@georgemccabe
georgemccabe marked this pull request as draft July 16, 2026 21:50
@georgemccabe
georgemccabe marked this pull request as ready for review July 17, 2026 15:49

@mkavulich mkavulich left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for all your work on this

@mkavulich
mkavulich merged commit da686ac into develop Jul 17, 2026
3 checks passed
@georgemccabe
georgemccabe deleted the feature/regression_tests branch July 17, 2026 20:15
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.

2 participants