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

Move Regression Testing in to the CI #2390

Merged
merged 44 commits into from
Apr 12, 2023
Merged

Move Regression Testing in to the CI #2390

merged 44 commits into from
Apr 12, 2023

Conversation

JacksonBurns
Copy link
Contributor

@JacksonBurns JacksonBurns commented Mar 2, 2023

Final Updates before review (cd69831) (7 April 2023):

Below is a complete list of all changes made in this PR as well as the reasoning for each change. I believe this will completely replace the existing RMG-tests structure without loss of any functionality, and also adds functionality with (1) scheduled testing and (2) upload of detailed files for failed runs for easier analysis.

Changes, from least complex to most:

  • Added a new directory test/regression (future PRs, see Breaking Change on Aug. 1: Switch from nosetest to pytest and Overhaul Testing Layout #2380, will move other types of tests into test)
    • Added the regression testing files from RMG-tests into this directory. They are identical to those in RMG-tests
  • Added all files in test/regression to the .gitignore except the above files to avoid anyone accidentally committing output from the regression tests when running locally
  • Deleted .travis.yml since we no longer use travis
  • Removed badges from README.md which refer to travis, codacy (which is no longer used), and gitter (also no longer used)
  • Removed unused lines from Makefile
    • All the unit tests had checks for Windows, which are no longer needed since we don't support windows
    • The example 4 (which used MOPAC) had a setup step for travis
  • Deleted trigger-rmg-tests.sh since the regression testing is now done on this repository
  • Extensive updates to CI.yml to improve performance, increase maintainability, and add regression testing
    • Header added, with changelog
    • Comments added throughout file to explain what each command does
    • CI will now be run on a schedule and on pushes to any branch, but not pull requests (the CI will still be required to pass before merging)
    • Actions Concurrency added (see Prevent Orphaned GitHub actions from running with Actions Concurrency #2385)
    • Bumped checkout to v3
    • No longer clone Q2DTor, which is not used
    • Install step changed so that the Julia install will actually work and allow regression tests to execute
    • Multiple steps to run regression tests, upload results for reference, and compare to baseline (see below for a description of what CI now does)

High-level summary of how the CI now works:

  1. Someone pushes a commit to a branch, either (i) a merge commit to main or (ii) a developer working on a non-main branch
  2. Checkout the code and build a working RMG
  3. Run the unit tests, functional tests, and database tests
  4. Execute the examples which are used for regression testing
    • If execution fails, upload the results so developer can debug
  5. If (i) upload the results of this execution for future runs to use as reference else (ii) download the most recent results and compare them (this is the actual regression testing step)

Basically, every day or whenever we merge to main we run all the regression examples and save the results. Whenever someone makes a commit to a non-main branch, we attempt to run the regression examples and compare it to this saved result.

Because this has dramatically simplified the procedure of regression testing and we now only build the environment once, total execution time is less than half of RMG-tests. We also can now actually change the environment and compare regression results - RMG-tests did not allow this.

Before merging, we will need to un-comment the indicated step where the results are retrieved. This will only work once the changes are merged into main. This means that after approval we will have to make an additional commit that would typically require re-approval, but we will need to bypass that requirement.

Original PR description continues below:

Move the Regression testing from RMG-tests to RMG-Py, as described here.

Updates as of commit 9718ae1 (5 April 2023):

Changes to CI.yml

  • general cleanup:
    1. removal of meaningless lines
    2. adding concurrency checking
    3. adding a schedule
    4. adding docstrings
  • run it only on push to a branch, not on PR which previously caused runs to be duplicated
  • after completing the Julia install, the regression tests are executed and then the results are either:
    1. compared to the baseline if the CI is running on a development branch or
    2. uploaded to GitHub for future comparisons if scheduled (which is the baseline)

Changes to the repo in general:

  • delete trigger_rmg_tests.sh since it is not longer used
  • remove unused lines in Makefile related to use on windows (which is no longer supported) and with travis (which is also no longer in use)
  • remove outdated badges from the README
  • add the regression tests from RMG-Tests that are used by the CI

I believe this action will need to be copied into RMG-Database to match the behavior of RMG-Tests being triggered on commits to either RMG-Py or RMG-Database

@JacksonBurns JacksonBurns added Type: Testing Status: WIP This is currently work-in-progress labels Mar 2, 2023
@JacksonBurns JacksonBurns self-assigned this Mar 2, 2023
@codecov
Copy link

codecov bot commented Mar 3, 2023

Codecov Report

Merging #2390 (cd69831) into main (0579e03) will decrease coverage by 0.20%.
The diff coverage is n/a.

❗ Current head cd69831 differs from pull request most recent head 7dce868. Consider uploading reports for the commit 7dce868 to get more accurate results

@@            Coverage Diff             @@
##             main    #2390      +/-   ##
==========================================
- Coverage   48.31%   48.11%   -0.20%     
==========================================
  Files         110      110              
  Lines       30654    30650       -4     
  Branches     7994     7995       +1     
==========================================
- Hits        14810    14748      -62     
- Misses      14301    14366      +65     
+ Partials     1543     1536       -7     

see 4 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@JacksonBurns JacksonBurns force-pushed the do_regression_with_ci branch from 2433840 to d648fd8 Compare April 1, 2023 12:50
@JacksonBurns JacksonBurns added Status: Ready for Review PR is complete and ready to be reviewed and removed Status: WIP This is currently work-in-progress labels Apr 7, 2023
hwpang
hwpang previously approved these changes Apr 7, 2023
Copy link
Contributor

@hwpang hwpang left a comment

Choose a reason for hiding this comment

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

@JacksonBurns and I met in person today and he explained the whole PR to me, and it looks good to me. It's probably better to have someone who is more familiar with the RMG-tests like @mjohnson541 to review it before we merge. We should also remember to rebase & uncomment some parts of the code before we merge.

@JacksonBurns JacksonBurns force-pushed the do_regression_with_ci branch from 2ea5881 to 7dce868 Compare April 12, 2023 20:57
@hwpang
Copy link
Contributor

hwpang commented Apr 12, 2023

We had a discussion today at RMG subgroup, and we had Bill's agreement to merge this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Ready for Review PR is complete and ready to be reviewed Type: Testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent Orphaned GitHub actions from running with Actions Concurrency
2 participants