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.

Sign up for GitHub

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

Finish [SuppressTempFileChecks] attribute functionality, #898 #1084

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft

Finish [SuppressTempFileChecks] attribute functionality, #898 #1084

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
e0ae7bf
Finish [SuppressTempFileChecks] attribute functionality, #898
paulirwin Jan 2, 2025
5ca12ae
PR feedback
paulirwin Jan 5, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 15 additions & 11 deletions src/Lucene.Net.TestFramework/Util/LuceneTestCase.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -440,7 +440,6 @@ public SuppressCodecsAttribute(params string[] value)
public string[] Value { get; private set; }
}

// LUCENENET TODO: Finish implementation
/// <summary>
/// Marks any suites which are known not to close all the temporary
/// files. This may prevent temp files and folders from being cleaned
Expand Down Expand Up @@ -723,11 +722,11 @@ private static IList<string> LoadCoreDirectories()
/// </summary>
internal static TestRuleSetupAndRestoreClassEnv ClassEnvRule { get; } = new TestRuleSetupAndRestoreClassEnv();

// LUCENENET TODO
// LUCENENET TODO: Implement these rules
/// <summary>
/// Suite failure marker (any error in the test or suite scope).
/// </summary>
public static readonly /*TestRuleMarkFailure*/ bool SuiteFailureMarker = true; // Means: was successful
public static /*TestRuleMarkFailure*/ bool SuiteFailureMarker = true; // Means: was successful

///// <summary>
///// Ignore tests after hitting a designated number of initial failures. This
Expand Down Expand Up @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, thanks for the clarification. I thought this was only per test.

Copy link
Contributor

Choose a reason for hiding this comment

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

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 a SuccssException. So, the TestRuleMarkFailure logic won't work as-is.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.


string message =
$$"""
{{result.Message}}
Expand Down Expand Up @@ -3086,9 +3089,10 @@ private static void RegisterToRemoveAfterSuite(FileSystemInfo f)
[MethodImpl(MethodImplOptions.NoInlining)]
protected string GetFullMethodName([CallerMemberName] string memberName = "")
{
return string.Format("{0}+{1}", this.GetType().Name, memberName);
return $"{this.GetType().Name}+{memberName}";
}

// LUCENENET specific - this is equivalent to TemporaryFilesCleanupRule in Lucene
private static void CleanupTemporaryFiles()
{
// Drain cleanup queue and clear it.
Expand Down Expand Up @@ -3126,19 +3130,19 @@ private static void CleanupTemporaryFiles()
}
catch (Exception e) when (e.IsIOException())
{
// Type suiteClass = RandomizedContext.Current.GetTargetType;
// if (suiteClass.IsAnnotationPresent(typeof(SuppressTempFileChecks)))
// {
Console.Error.WriteLine("WARNING: Leftover undeleted temporary files " + e.Message);
return;
// }
if (RandomizedContext.CurrentContext.CurrentTest.TypeInfo?.Type.GetCustomAttribute<SuppressTempFileChecksAttribute>(inherit: true) is { } suppressAttr)
{
Console.Error.WriteLine($"WARNING: Leftover undeleted temporary files (bugUrl: {suppressAttr.BugUrl}): {e.Message}");
paulirwin marked this conversation as resolved.
Show resolved Hide resolved
return;
}
throw;
}
}
else
{
if (tempDirBasePath != null)
{
Console.Error.WriteLine("NOTE: leaving temporary files on disk at: " + tempDirBasePath);
Console.Error.WriteLine($"NOTE: leaving temporary files on disk at: {tempDirBasePath}");
}
}
}
Expand Down
Loading