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

run_WE2E_tests.sh does not run the intended tests on Hera if COMPILER is not explicitly set #571

Closed
mkavulich opened this issue Feb 1, 2023 · 0 comments · Fixed by #637
Labels
bug Something isn't working

Comments

@mkavulich
Copy link
Collaborator

Expected behavior

The run_WE2E_tests.sh script should check to see if the following pre-defined file lists exist in the directory ufs-srweather-app/tests/WE2E/machine_suites/, in order of priority:

  1. ${test_type}.${machine}.${compiler}.nco
  2. ${test_type}.${machine}.${compiler}.com
  3. ${test_type}.${machine}.${compiler}
  4. ${test_type}.${machine}
  5. ${test_type}

So on Hera, running the fundamental tests with no other flags should run the tests found in the file fundamental.hera.intel.nco, since that file exists in machine_suites/ and it is first in the priority list. If the user specifies "--compiler=gnu", then the tests in fundamental.hera.gnu.com should be run.

Current behavior

With commit d03e646 (PR #477), the logic for checking the set of "fundamental" or "comprehensive" tests to run has been broken. The current logic assumes that "compiler" has been set, but if the user has not provided the argument then the default value will not have been filled in yet. So running the fundamental tests on, for example, Hera without specifying the compiler will default to the set of tests listed in machine_suites/fundamental, rather than the intended machine_suites/fundamental.hera.intel.nco, because ${compiler} will resolve as an empty string, meaning the logic looks for fundamental.hera..nco, which does not exist. Likewise, fundamental.hera..nco, fundamental.hera., and fundamental.hera do not exist, so finally the logic settles on the tests found in machine_suites/fundamental.

If compiler=intel or compiler=gnu is explicitly set, the correct set of tests is used.

Machines affected

This logic will be broken if compiler is not explicitly set for any platform with a compiler-specific test suite for the default compiler; currently this is only Hera.

Steps To Reproduce

  1. On Hera, run the WE2E tests without using the compiler= flag:

  2. Observe that the set of tests run is incorrect:

After processing the user-specified list of WE2E tests to run, the number 
of tests to run (num_tests_to_run) is
  num_tests_to_run = 9
and the list of WE2E tests to run (one test per line) is
  'grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_suite_GFS_v15p2'
  'grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_FV3GFS_suite_GFS_v16'
  'grid_RRFS_CONUS_25km_ics_FV3GFS_lbcs_RAP_suite_HRRR'
  'grid_RRFS_CONUS_25km_ics_GSMGFS_lbcs_GSMGFS_suite_GFS_v15p2'
  'grid_RRFS_CONUScompact_25km_ics_HRRR_lbcs_HRRR_suite_HRRR'
  'grid_RRFS_CONUScompact_25km_ics_HRRR_lbcs_HRRR_suite_RRFS_v1beta'
  'grid_RRFS_CONUScompact_25km_ics_HRRR_lbcs_RAP_suite_HRRR'
  'grid_RRFS_CONUScompact_25km_ics_HRRR_lbcs_RAP_suite_RRFS_v1beta'
  'nco_grid_RRFS_CONUScompact_25km_ics_HRRR_lbcs_RAP_suite_HRRR'

This is the list of tests specified in machine_suites/fundamental, rather than the intended tests in machine_suites/fundamental.hera.intel.nco.

Detailed Description of Fix (optional)

The logic filling in default values for input arguments should be moved earlier to before this logic.

The new python-based script (#558) does not suffer from this error, so when that is adopted this problem will go away.

Additional Information

Note: As I have previously mentioned, Il do not think that this testing strategy is a good idea. The "fundamental" set of tests should be the same on each platform for consistency, and it should be a small subset of tests checking the "most important" functionality of the workflow to ensure that nothing major has been broken, regardless of which platform is used for development/testing. But I felt it was important to point out that the intended functionality is currently broken.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant