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

Friendly test names #466

Merged
merged 10 commits into from
Dec 17, 2019
Merged

Conversation

spottedmahn
Copy link
Contributor

@spottedmahn spottedmahn commented Jul 27, 2018

@spottedmahn
Copy link
Contributor Author

40891919-bebb29cc-675c-11e8-913f-483183b8e34d
40891965-9c143ad4-675d-11e8-9e93-ff110f823cea

@spottedmahn
Copy link
Contributor Author

Clicking on the details here:
image

Yields:
image

@spottedmahn
Copy link
Contributor Author

Hi @jayaranigarg - any thoughts? thanks!

@jayaranigarg
Copy link
Member

jayaranigarg commented Aug 16, 2018

@spottedmahn : Apologies for the delay. I have added few comments. Please look into them. Can you please add UnitTests as well for these changes?

Also, clicking on 'Details' showing Not Found seems like a temporary issue which got fixed now.

@jayaranigarg
Copy link
Member

@dotnet-bot : Test this please

@parrainc
Copy link
Contributor

Guys, this one looks like it has been sit here for a while.
@jayaranigarg is this PR still valid? meaning that if everything gets up to date, and complete what's missing, could it be merged and part of next package release (maybe)?

If so, @spottedmahn is there anything I can help you with to complete this PR? If you don't have time to finish it, would you mind providing me with write-access to your remote branch, so I can push whatever is missing? Or, I could create a fork from it, and submit a new PR with your changes and what's missing. Let me know...

@spottedmahn
Copy link
Contributor Author

Hi @parrainc 👋

write-access to your remote branch

Not sure how to do that. Wonder if that is only a thing with organizations? Got a URL you can share on how to do that?

@spottedmahn
Copy link
Contributor Author

Hi @parrainc 👋

Can you please add UnitTests as well for these changes?

Need to tackle that...

@parrainc
Copy link
Contributor

Not sure how to do that. Wonder if that is only a thing with organizations? Got a URL you can share on how to do that?

sure, you can follow this guide in order to provide me with access to your repo. This is needed so I can update this PR by pushing code to your repo, instead of having to open another PR from this.

Need to tackle that...
As soon as I can push to the repo, I'll tackle that and whatever is needed.

@spottedmahn
Copy link
Contributor Author

spottedmahn commented Aug 16, 2019

sure, you can follow this guide in order to provide me with access to your repo

Oh, that was easy 👍, done ✅. How didn't I see that 🤦‍♂️?!

@spottedmahn
Copy link
Contributor Author

Hi @jayaranigarg - any updates for us? Thanks

@parrainc
Copy link
Contributor

parrainc commented Dec 4, 2019

Hi @jayaranigarg - any updates for us? Thanks

I think it's been ready for a while now. Could be merged any time when possible, @jayaranigarg

@nohwnd nohwnd self-assigned this Dec 17, 2019
Fixed tests to prove that UnitTestElement is setting the DisplayName and
that it does not use the Display name as the method name. Using Display
Name as method name leads to searching for methods like "Class1.Display Name"
which won't be found and the test runner will fail.
@nohwnd
Copy link
Member

nohwnd commented Dec 17, 2019

@AbhitejJohn Fixed and seems good to go. The fixed tests actually prove on unit test level the thing that was causing it to fail only in acceptance. Is there more code review needed?

Attaching the project I used to figure out the failures in case someone comes back to this in the future, or in case more changes are needed from my side.

UnitTestProject3.zip

@nohwnd
Copy link
Member

nohwnd commented Dec 17, 2019

Looks like ReflectHelper has some problem, fixing that.

The usage of ReflectHelper is a bit nuanced, the generic
method I used originally uses a static method interallly
which prevents it from being mocked. So in order to write
the tests in the same way as the other tests are written
I use the mock to act like the method is decorated with the
correct attribute, and need to use a different method internally
but at least the generic method is not the sigle outlier in that file.
@nohwnd nohwnd merged commit d638ad8 into microsoft:master Dec 17, 2019
@spottedmahn
Copy link
Contributor Author

Status: merged 🎉🎉

So when does this become available? Is this delivered via the MSTest.TestAdapter NuGet package? Do you guys label PRs w/ a release number?

@nohwnd
Copy link
Member

nohwnd commented Dec 17, 2019

Dunno all the details, I am on this team for just a few days, but my next step is to learn how to release this as a preview to nuget, so expect it soonish 😁

@AbhitejJohn
Copy link
Contributor

Thanks @spottedmahn and @nohwnd for getting this through. Daily builds should be available on the myget feed linked in the repo's readme. However we'd definitely want to put this out to nuget as a preview package post the regular validation so this can be consumed.

@spottedmahn
Copy link
Contributor Author

Daily builds should be available on the myget feed linked in the repo's readme

Great! Did we change the adapter or the framework package? Seems like the adapter...

@AbhitejJohn
Copy link
Contributor

@spottedmahn : That would be both the framework and the adapter because we had the TestMethod attribute change. It doesn't look like the latest build has your changes just yet. I'm hoping we should have a package tomorrow.

@spottedmahn
Copy link
Contributor Author

That would be both the framework and the adapter because we had the TestMethod attribute change

Oh, cool, thanks!

I'm hoping we should have a package tomorrow.

🤞

@nohwnd
Copy link
Member

nohwnd commented Dec 18, 2019

@spottedmahn Gotchu!

https://www.nuget.org/packages/MSTest.TestFramework/2.1.0-beta2
https://www.nuget.org/packages/MSTest.TestAdapter/2.1.0-beta2

@spottedmahn
Copy link
Contributor Author

Testing out 2.1.0-beta2, created a few issues:

@ahmedalejo
Copy link

ahmedalejo commented May 1, 2020

Just making sure everyone sees this solution for #644

Found a better and simpler extension point that could be baked into TestMethodAttribute with ease.

This is what i´m currently using:

cheers.

public class PrettyTestClassAttribute : TestClassAttribute
{
    public override TestMethodAttribute GetTestMethodAttribute(TestMethodAttribute wrappedTestMethodAttribute)
    {
        var attribute = base.GetTestMethodAttribute(wrappedTestMethodAttribute);

        return attribute as PrettyTestTestMethodAttribute ?? new PrettyTestTestMethodAttribute(attribute);
    }
}

public class PrettyTestMethodAttribute : TestMethodAttribute
{
    private readonly TestMethodAttribute wrappedTestMethodAttribute;

    public PrettyTestMethodAttribute(){ }

    public PrettyTestMethodAttribute(TestMethodAttribute wrappedTestMethodAttribute) =>
        this.wrappedTestMethodAttribute = wrappedTestMethodAttribute;

    public override TestResult[] Execute(ITestMethod testMethod)
    {
         TestResult[] results = testMethodAttribute is null
             ? base.Execute(testMethod)
             : testMethodAttribute.Execute(testMethod);

        if(results.Any())
            results[0].DisplayName = testMethod.TestMethodName
                .Replace("_eq_", " == ")
                .Replace("_neq_", " != ")
                .Replace("_gt_", " > ")
                .Replace("_gte_", " >= ")
                .Replace("_lt_", " < ")
                .Replace("_lte_", " <= ")
                .Replace("Bug_", "🐞")
                .Replace("_", " ");
        return results;
    }
}

Hopefully will create a new pull request suggesting to bake this into TestMethodAttribute

This was referenced Mar 16, 2021
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.

None yet

6 participants