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

Potential bug could occur in deletion of files in test harness #23749

Open
permcody opened this issue Mar 15, 2023 · 2 comments
Open

Potential bug could occur in deletion of files in test harness #23749

permcody opened this issue Mar 15, 2023 · 2 comments
Labels
C: TestHarness Good first issue T: task An enhancement to the software.

Comments

@permcody
Copy link
Member

permcody commented Mar 15, 2023

Reason

We've had an input parameter in the TestHarness Testers hierarchy for several years that will skip the normal deletion of expected output files (from that test) prior to running that test. This parameter exists mostly for the case where an output may need to be used in multiple tests or is expensive to generate more than once, etc.

The issue is that in extreme circumstances, never clearing out output files can obscure bugs where the output is no longer being generated but the file exists so it's not being detected by the TestHarness.

For the most part, this condition is safely mitigated by the continuous integration system since we start with a clean checkout before running the tests so leftovers cannot feed into the testing process. However, there is a remote chance that not cleaning these out in the development environment at least occasionally could lead to issues that seem to only arise in the CI system.

Design

We already had a parameter called delete_output_before_running in the RunApp class (probably should be in the FileTester class, but that's where it's currently located). When the parameter is set to false, we normally don't delete the expected outputs before running. I'm proposing that we modify that logic slightly so that when that parameter is set, we delete output after the test runs instead of before. This shouldn't have any effect on any tests we have in the system now. Altenatively, we could create an enumeration instead of a Boolean: delete_output = BEFORE | AFTER. Defaulting to before for the 99% case. Again, the thinking is that in the rare cases where we don't want to delete before, it's because we are working with an expensive to generate file or one where we want to run a separate tester to process. In those rare cases, cleaning up the output after that tester could help alleviate any problems one might encounter.

Design 2: A completely different design might be to delete extra files before running the first part of a multi-part test:

  • Test A generates output "out_a"
  • Test B processes output "out_a" so we can't delete that file before processing it (current behavior).

Test A could be modified to delete "out_a" something it likely doesn't do automatically since it usually isn't processing "out_a", we could just make this explicit... but this could be error prone.

Impact

Very minor - but falls into the "don't say I didn't think about this super obscure error that really should never bite anyone, but then of course does because you know... chaos is a b****"

@permcody permcody added T: task An enhancement to the software. Good first issue C: TestHarness labels Mar 15, 2023
@AbdullahAlmanei
Copy link

Hello! I'd love to attempt to help out here. My current idea is to check if delete_output_before_running is set to False at the end of running the tests. If false, we delete the files. This way, normal functionality is not affected, and test files are only deleted after tests are run. This seems like it would be the least disruptive solution. Please let me know if you have any comments.

I'd really appreciate it if you could point out where in the project tests are run, so that I may add the test I mentioned above.

@permcody
Copy link
Member Author

permcody commented May 4, 2023

Hi Abdullah,

Yes, your suggestion is pretty close to what I suggested. However, the scope of my suggestion is much more focused. For the vast majority of tests, we don't need to clean up the files after the tests have been run. In fact, that's much, much harder than it sounds as users are free to do whatever they want in terms of generating files so the only sure fire way of making sure you delete them all is to take a snapshot of the filesystem before and after your test to know if you've got them all. That's essentially why git clean is so effective. It's either tracked by the repository or not. With my suggestion, I'm only suggesting that we delete files "afterwards" for tests where this parameter is explicitly set. That's it!

As to where this is occuring, grep is your friend. If you have a local copy of MOOSE just search through the tree looking at all the test specification files (normally called "tests") and grep for that parameter specifically. Then you can run just that test and see how it currently behaves. My suggestion would be to remove those files after the test completes, but again only for these types of tests.

Let me know if you are still interested in working this problem, the changes shouldn't be too difficult you'll find the relevant logic in the TestHarness "Testers" directory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: TestHarness Good first issue T: task An enhancement to the software.
Projects
None yet
Development

No branches or pull requests

2 participants