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

Removed Discover Warnings in Test Output | Issue #437 #480

Merged
merged 6 commits into from
Oct 23, 2018

Conversation

parrainc
Copy link
Contributor

@parrainc parrainc commented Aug 25, 2018

This PR fixes the issue #437. After building the solution and the test discover starts, non-public tests won't report in the Test output.

[Update]
Created a "dummy test class attribute" that inherit from TestClassAttribute to be used in the dummy test classes that were generating warnings in the Output.

This is how is looks after these changes:
after-2
after-1

@parrainc parrainc changed the title Removed Discover Warnings in Test Output | #437 Removed Discover Warnings in Test Output | Issue #437 Aug 25, 2018
@jayaranigarg
Copy link
Member

@parrainc : Thank you for your contribution but unfortunately we want to have test classes to work only with public access modifier and not with private/internal for performance reasons.

@parrainc
Copy link
Contributor Author

Hi @jayaranigarg thanks for your comment. But, notice that the behaviour will be the same for discovering tests (just public test classes), the only thing I changed is that internal/private test classes won't report in the output, as stated the expected behaviour in the issue.

In any case, if you have suggestions that I can follow, so I can update the PR, please let me know.

@parrainc
Copy link
Contributor Author

parrainc commented Sep 7, 2018

@jayaranigarg just following this one up... wanted to know if there's suggestions on what needs to be modified in this PR.

@jayaranigarg
Copy link
Member

@parrainc : We indeed want internal/private classes to be reported in the output, otherwise a user will not be able to figure out the reason why his/her tests are not getting discovered.

We somehow just want to suppress these warnings for our unit-test classes, where we don't want testmethods to run as such as they are dummy classes which are merely used as assets in writing unit tests.

@parrainc
Copy link
Contributor Author

Gotcha! I misunderstood the issue a little bit. @jayaranigarg
I'm updating this PR with new commit suppressing these warnings instead of just removing the reporting functionality on internal/private classes.

@jayaranigarg
Copy link
Member

@parrainc : That's great! Let us know once you push in the new changes.

@parrainc
Copy link
Contributor Author

@jayaranigarg new changes are ready for review (with some delay). Please take a look when available and let me know your thoughts :)

@@ -412,6 +412,10 @@ public void TestMethod()
}
}

private class DummyTestClassAttribute : UTF.TestClassAttribute
Copy link
Member

Choose a reason for hiding this comment

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

Please add [DummyTestClass] to class DummyTestClassWithCleanupMethods(Line 388) as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just did it, but when added the [DummyTestClass] attribute to DummyTestClassWithCleanupMethods class, one of the unit test failed RunCleanupShouldReturnCleanupResultsForAssemblyAndCla ssCleanupMethods because of the AssemblyCleanup method is not being called.

Copy link
Member

Choose a reason for hiding this comment

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

That is weird.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed it is. I did even try debugging the unit test that was using the DummyTestClassWithCleanupMethods with the [DummyTestClass] and [UTF.TestClass] but could not observe nothing. and got same behavior except for the part of the assemblyCleanup thing. Notice that when invoking the ClassCleanup it works properly.

Copy link
Member

Choose a reason for hiding this comment

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

Tried debugging this. Basically, when we get DeclaredMethods for DummyTestClassWithCleanupMethods, it is not returning AssemblyCleanup() method in the list of methods for that type and hence AssemblyCleanup() is not getting called. Now, the question is why AssemblyCleanup() is not present in the list when we mention [DummyTestClass] and is present in case of [UTF.TestClass]

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's the same question I asked to myself...
I created a mstest project, added a class CustomTestClass that inherits from TestClass, and set up the assembly/class initalize and cleanup methods. It looks like there could be an issue when a test class is inheriting from a custom attribute that inherits from TestClassAttribute.
Have you seem any issues regarding to that?

Here's the debug output with CustomTestClass attribute:
expectedValue is a private static int field declared in the test class

ClassInitialize: before -> 0
ClassInitialize: after -> 321
TestMethod: expectedValue -> 321

Assert.AreEqual failed. Expected:<123>. Actual:<321>.

ClassCleanup: expectedValue -> 321

notice that with the custom attribute, neither the assembly initialize nor the cleanup were executed

Here's the debug output with TestClass attribute:

AssemblyInitialize: before -> 0
AssemblyInitialize: after -> 123
ClassInitialize: before -> 123
ClassInitialize: after -> 123
TestMethod: expectedValue -> 123
ClassCleanup: expectedValue -> 123
AssemblyCleanup: expectedValue -> 123

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jayaranigarg any updates?
I'm thinking of opening an issue with my inputs in above comments? Would that be alright?

Copy link
Member

Choose a reason for hiding this comment

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

@parrainc : That should be fine. Please raise a separate issue for this and let's get this PR in.

@jayaranigarg
Copy link
Member

@parrainc : LGTM 👍 Just a minor comment. Please address that and it should be good to push in.

@jayaranigarg jayaranigarg merged commit 2be8110 into microsoft:master Oct 23, 2018
@parrainc parrainc deleted the issue437-discoverwarnings branch October 23, 2018 14:10
@parrainc parrainc mentioned this pull request Oct 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants