Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
Next Next commit
Today even if classInit wasnt called we used to call class cleanup. f…
…ixing that and adding a test to cover the scenario
  • Loading branch information
siddhap committed Feb 19, 2018
commit 1ebe2ca634f6bb0a3f7a89f64be5cdf97c25c5c1
75 changes: 46 additions & 29 deletions src/Adapter/MSTest.CoreAdapter/Execution/TestClassInfo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -103,10 +103,15 @@ internal set
}

/// <summary>
/// Gets a value indicating whether is class initialize executed.
/// Gets a value indicating whether class initialize has executed.
/// </summary>
public bool IsClassInitializeExecuted { get; internal set; }

/// <summary>
/// Gets a value indicating whether class cleanup has executed.
/// </summary>
public bool IsClassCleanupExecuted { get; private set; }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is this required?


/// <summary>
/// Gets the exception thrown during <see cref="ClassInitializeAttribute"/> method invocation.
/// </summary>
Expand Down Expand Up @@ -306,40 +311,52 @@ public string RunClassCleanup()
return null;
}

lock (this.testClassExecuteSyncObject)
if (this.IsClassInitializeExecuted)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: how about this.ClassCleanupMethod == null || this.IsClassInitializeExecuted == false?

{
try
{
this.ClassCleanupMethod.InvokeAsSynchronousTask(null);

return null;
}
catch (Exception exception)
lock (this.testClassExecuteSyncObject)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lock can be removed? All clean ups done in single thread. /cc @AbhitejJohn

{
var realException = exception.InnerException ?? exception;

string errorMessage;

// special case AssertFailedException to trim off part of the stack trace
if (realException is AssertFailedException ||
realException is AssertInconclusiveException)
{
errorMessage = realException.Message;
}
else
if (this.IsClassInitializeExecuted)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IsClassInitializeExecuted [](start = 29, length = 25)

why another check needed inside?

{
errorMessage = StackTraceHelper.GetExceptionMessage(realException);
}
try
{
this.ClassCleanupMethod.InvokeAsSynchronousTask(null);

return string.Format(
CultureInfo.CurrentCulture,
Resource.UTA_ClassCleanupMethodWasUnsuccesful,
this.ClassType.Name,
this.ClassCleanupMethod.Name,
errorMessage,
StackTraceHelper.GetStackTraceInformation(realException)?.ErrorStackTrace);
return null;
}
catch (Exception exception)
{
var realException = exception.InnerException ?? exception;

string errorMessage;

// special case AssertFailedException to trim off part of the stack trace
if (realException is AssertFailedException ||
realException is AssertInconclusiveException)
{
errorMessage = realException.Message;
}
else
{
errorMessage = StackTraceHelper.GetExceptionMessage(realException);
}

return string.Format(
CultureInfo.CurrentCulture,
Resource.UTA_ClassCleanupMethodWasUnsuccesful,
this.ClassType.Name,
this.ClassCleanupMethod.Name,
errorMessage,
StackTraceHelper.GetStackTraceInformation(realException)?.ErrorStackTrace);
}
finally
{
this.IsClassCleanupExecuted = true;
}
}
}
}

return null;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,18 @@ public void TestClassInfoClassCleanupMethodSetShouldThrowForMultipleClassCleanup
ActionUtility.ActionShouldThrowExceptionOfType(action, typeof(TypeInspectionException));
}

[TestMethod]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is a negative test case. Can we add a positive test case as well?

public void TestClassInfoClassCleanupMethodShouldNotInvokeWhenNoTestClassInitializedIsCalled()
{
this.testClassInfo.ClassCleanupMethod = this.testClassType.GetMethods().First();
this.testClassInfo.ClassInitializeMethod = this.testClassType.GetMethods()[1];

var ret = this.testClassInfo.RunClassCleanup(); // call cleanup without calling init

Assert.AreEqual(null, ret);
Assert.AreEqual(false, this.testClassInfo.IsClassCleanupExecuted);
}

[TestMethod]
public void TestClassInfoHasExecutableCleanupMethodShouldReturnFalseIfClassDoesNotHaveCleanupMethod()
{
Expand Down