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

updating RegressionTest generateAnswers for when two classes are used for test .dat & answer files #821

Merged

Conversation

sambish5
Copy link
Collaborator

This updates generateAnswers to be able to generate answer files in the correct location when getMyTestParameterFiles is passed two separate classes. This allows files to generate/regenerate in the provided answer class, based on .dat files in the other class directory.

@sambish5 sambish5 added the test-only The change only impacts test code label Jun 17, 2024
@sambish5 sambish5 requested review from rpg36 and fbruton June 17, 2024 15:17
@sambish5 sambish5 self-assigned this Jun 17, 2024
Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

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

This is fundamentally broken - in fact, we'll also need to rework the related changes in the UnitTest class because these changes aren't thread-safe, and could corrupt results when concurrently testing multiple classes that extend UnitTest or ExtractionTest. We can't use a static member in the base class to "store" the answer file paths.

h/t to @jdcove2 for noticing and pointing this out.

@sambish5 sambish5 force-pushed the generateAnswers-getTestParams branch from 5580001 to abaa765 Compare June 25, 2024 19:43
@sambish5 sambish5 marked this pull request as ready for review June 25, 2024 19:43
@sambish5
Copy link
Collaborator Author

updated based on the new thread-safe version of UnitTest (#826) that allows for different directories used for test data/answer files

@sambish5 sambish5 changed the title updating RegressionTest generateAnswers for when two classes are provided in getMyTestParameterFiles updating RegressionTest generateAnswers for when two classes are used for test .dat & answer files Jun 25, 2024
Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

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

Still susceptible to race conditions across tests that extend RegressionTest

Copy link
Collaborator

@drivenflywheel drivenflywheel left a comment

Choose a reason for hiding this comment

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

Working as intended when locally testing integration

Copy link
Collaborator

@jdcove2 jdcove2 left a comment

Choose a reason for hiding this comment

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

I tested these changes against a follow-on system and it worked as expected.

@jpdahlke jpdahlke added this to the v8.6.0 milestone Jun 26, 2024
@jpdahlke jpdahlke merged commit 70b319a into NationalSecurityAgency:main Jun 26, 2024
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test-only The change only impacts test code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants