Finish [SuppressTempFileChecks] attribute functionality, #898 #1084
Conversation
| @@ -938,6 +937,10 @@ public virtual void TearDown() | |||
|
|
|||
| if (result.ResultState == ResultState.Failure || result.ResultState == ResultState.Error) | |||
| { | |||
| // LUCENENET specific - Track that at least one test in the suite failed. | |||
| // LUCENENET TODO: finish implementation of RuleChains so this is not needed (#1088) | |||
| SuiteFailureMarker = false; | |||
There was a problem hiding this comment.
There is nothing resetting this back to true, so the first failure will cascade to the other tests. I think we were better off with this hard coded to true because this will leave lots of temp files on the disk for passing tests.
There was a problem hiding this comment.
That is indeed the point. It tracks whether there have been any failures. In Lucene, suiteFailureMarker is static and is flagged as failed if any tests fail.
There was a problem hiding this comment.
Oh, thanks for the clarification. I thought this was only per test.
There was a problem hiding this comment.
According to ChatGPT, that isn't how that works. It resets the state at the beginning of every test: https://chatgpt.com/share/677af21b-bcb8-8005-ad95-b070d0c6cdaa.
But whatever the case, I am hesitant to merge this to master when a test failure could leave all of the temporary files on disk for every successful test in the assembly that runs after it. There is a specific setting for that: tests:leavetmpdir.
Side note: In NUnit, a failure isn't always an exception and
Assert.Pass()throws aSuccssException. So, theTestRuleMarkFailurelogic won't work as-is.
There was a problem hiding this comment.
I really debated whether we should even be doing this anyways in the issue comments. Again, no Lucene tests even use this attribute. I'll let you take ownership of this one from here because I'm losing interest in the purity aspect of getting this working when there is no actual benefit to our code. You can feel free to use whatever of this PR might be useful in your other test framework changes and I'm just going to mark this as draft for now. I'd appreciate getting some of my other PRs reviewed ahead of this one, anyways.
Finishes implementation of the
[SuppressTempFileChecks]attribute functionality.Fixes #898
Description
This attribute will cause the tear down logic to swallow any exceptions when cleaning up temporary files after the tests run. No tests actually need this attribute, but theoretically it is possible that a TestFramework user might want this.
This also fixes an issue where the previous code was treating all tests as if it had this attribute, and was not throwing if removing the temporary files failed. This change will make our tests more reliable by ensuring they fail if temp file handles are held during cleanup.
I'm also open to removing this attribute, since none of our tests need it and latest Lucene doesn't use it internally either. It could be viewed as a bad practice to just ignore these files being held too long.