-
Notifications
You must be signed in to change notification settings - Fork 253
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
Today even if classInit wasnt called we used to call class cleanup. f… #372
Conversation
…ixing that and adding a test to cover the scenario
errorMessage = realException.Message; | ||
} | ||
else | ||
if (this.IsClassInitializeExecuted) |
There was a problem hiding this comment.
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?
@@ -111,6 +111,18 @@ public void TestClassInfoClassCleanupMethodSetShouldThrowForMultipleClassCleanup | |||
ActionUtility.ActionShouldThrowExceptionOfType(action, typeof(TypeInspectionException)); | |||
} | |||
|
|||
[TestMethod] |
There was a problem hiding this comment.
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?
/// <summary> | ||
/// Gets a value indicating whether class cleanup has executed. | ||
/// </summary> | ||
public bool IsClassCleanupExecuted { get; private set; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this required?
return null; | ||
} | ||
catch (Exception exception) | ||
lock (this.testClassExecuteSyncObject) |
There was a problem hiding this comment.
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
@@ -306,40 +311,52 @@ public string RunClassCleanup() | |||
return null; | |||
} | |||
|
|||
lock (this.testClassExecuteSyncObject) | |||
if (this.IsClassInitializeExecuted) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
* Fixing Key collision for test run parameters (#328) * Base commit * Adding Unit Test * PR comments * Adding logs for diagnostics * Ignoring Test Temporarily * Removing ignore # Conflicts: # test/UnitTests/MSTest.CoreAdapter.Unit.Tests/Execution/TestExecutionManagerTests.cs * Fixing the copyright in the nuspec files. (#350) * Today even if classInit wasnt called we used to call class cleanup. f… (#372) * Today even if classInit wasnt called we used to call class cleanup. fixing that and adding a test to cover the scenario # Conflicts: # src/Adapter/MSTest.CoreAdapter/Execution/TestClassInfo.cs * [Symbol archiving]Converting Portable pdb to full (#375) * COnverting portable pdbs to full for Symbol archiving * Fixing nuget package path * Test fix * Indent fix * Update Fileversion for adapter and framework dlls (#337) * File version update * Moving to 15.6 * PR comments * More PR comments * Throwing exception only if it is of type TestPlatformFormatException (#332) * Throwing exception only if it is of type TestPlatformFormatException * test * Fail discovery if any exception thrown in IDiscoveryContext.GetTestCaseFilter * Fix masking assembly load faolure error message (#382) # Conflicts: # src/Adapter/MSTest.CoreAdapter/Discovery/AssemblyEnumeratorWrapper.cs
Today even if classInit wasnt called we used to call class cleanup. fixing that and adding a test to cover the scenario
#355