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

Reg-test scripts modification to help avoid race condition and cleanup of caselist #1244

Merged
merged 3 commits into from
Sep 7, 2022

Conversation

andrew-platt
Copy link
Collaborator

This PR is ready to be merged

Feature or improvement description
As discussed in PR #1199, there is a frequent issue with race conditions when copying test case inputs over to the regression testing location. This is caused by the shutil.copytree command that errors out if there are pre-existing files. To get around this issue, the rtl.copyTree command is now used.

Additionally, three test cases are dropped from the python interface testing and HydroDyn module testing. These cases are equivalent to ones that had been dropped from main OpenFAST test case list as they do not agree sufficiently between platforms for the new testing methodology (likely due to numerical issues in HydroDyn). These cases include:

  • Python interface test:
    • 5MW_ITIBarge_DLL_WTurb_WavesIrr_py
  • HydroDyn module tests:
    • hd_5MW_ITIBarge_DLL_WTurb_WavesIrr
    • hd_5MW_OC4Jckt_DLL_WTurb_WavesIrr_MGrowth

Related issue, if one exists
See comment #1199 (comment) and following discussion in PR #1199 for context on the rtl.copyTree.
The dropped cases should have been dropped with PR #1217.

Impacted areas of the software
This only affects the running of test cases.

Test results, if applicable
See above.

…OpenFAST tests that were removed.

- 5MW_ITIBarge_DLL_WTurb_WavesIrr_py
- hd_5MW_ITIBarge_DLL_WTurb_WavesIrr
- hd_5MW_OC4Jckt_DLL_WTurb_WavesIrr_MGrowth
This will help a bit in minimizing a race condition during test running. See comments in PR OpenFAST#1199
@andrew-platt andrew-platt added this to the v3.3.0 milestone Sep 6, 2022
@andrew-platt andrew-platt self-assigned this Sep 6, 2022
@andrew-platt
Copy link
Collaborator Author

This may partially fix an issue in testing for PR #1086.

@andrew-platt andrew-platt requested review from deslaughter and removed request for rafmudaf September 6, 2022 21:37
@deslaughter
Copy link
Collaborator

The rtest-interfaces action failed because the 5MW_Land_DLL_WTurb_cpp test couldn't find its yaml input file. I think that .yaml was one of the file types in the excludeExt list which was added to the Python files. Changing .yaml to .sum.yaml in the excludeExt list should fix the issue by excluding the regression test reference files, but still allowing the input file. However, this change would require modifying rtestlib.py to something other than os.path.splitext to get the extension. It may be better to remove .yaml from excludeExt in executeOpenfastCppRegressionCase.py.

While these changes will reduce the chances of the script failing due to simultaneous tests writing to the same file, they won't prevent it, as discussed in PR #1199.

@andrew-platt
Copy link
Collaborator Author

Thanks for looking at this! I agree this is not going to completely solve the race condition.

Copy link
Collaborator

@deslaughter deslaughter left a comment

Choose a reason for hiding this comment

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

Changes look good and all tests are passing.

@deslaughter deslaughter merged commit 0f8237e into OpenFAST:dev Sep 7, 2022
@andrew-platt andrew-platt deleted the f/DisableRegTests branch September 7, 2022 16:12
@rafmudaf rafmudaf mentioned this pull request Oct 27, 2022
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants