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

MSTest.TestAdapter/TestFramework 2.1.0 broke existing F# tests with a period in their name. #682

Closed
SomethingAwry opened this issue Feb 8, 2020 · 7 comments
Assignees
Milestone

Comments

@SomethingAwry
Copy link

SomethingAwry commented Feb 8, 2020

F# methods/functions are allowed to have special characters in their names by book-ending the name with two back-ticks. (Used for making friendly test names, etc.)

namespace NS
open Microsoft.VisualStudio.TestTools.UnitTesting
[<TestClass>]
type TC () =
    [<TestMethod>]
    member __.``a.b`` () : unit =
        ()

It seems that prior to 2.1.0, only the test explorer was messed up, showing a 'group' called a containing a test b, but with 2.1.0 it's now broken them: the bogus b test fails: Method NS.TC.b does not exist. (although the Test Detail Summary still shows the correct name of a.b)

#466 appears to have introduced a change that works by splitting on periods within testCase.FullyQualifiedName; perhaps that was the cause, though based on the existing test explorer behavior, perhaps it was something else?

@SomethingAwry SomethingAwry changed the title Friendly test names (pull request #466) broke existing F# tests with a period in their name. MSTest.TestAdapter/TestFramework 2.1.0 broke existing F# tests with a period in their name. Feb 8, 2020
@nohwnd
Copy link
Member

nohwnd commented Feb 8, 2020

I am sorry, that is very unfortunate that we did not catch this earlier. There was a lot of back and forth about how the name of the test should be figured out, and the splitting was one of the discussion points because at the last point only the whole name was available, but not the method name. So I am aware of what is causing this bug, I just don't know how to fix it.

Are you just reporting, or would you be willing to PR a fix for this?

@SomethingAwry
Copy link
Author

Just reporting; I only hit it shortly before, and perused code more to confirm suspicions (undone a little as I confirmed the test explorer issue was prior).

@slang25
Copy link
Contributor

slang25 commented Feb 8, 2020

I'm willing to take a look at this tonight and will report back

@nohwnd
Copy link
Member

nohwnd commented Feb 10, 2020

@slang25 it would be great if you added F# test project for this into acceptance tests (The project would go into testAssets folder) 🙂

@slang25
Copy link
Contributor

slang25 commented Feb 10, 2020

Got me a failing test, the fix wasn't as trivial as I'd hope, we'll need to track some extra info. Will continue tomorrow.

@nohwnd nohwnd added this to the 16.6.0 milestone Feb 13, 2020
@nohwnd nohwnd added the bug label Feb 13, 2020
@nohwnd
Copy link
Member

nohwnd commented Feb 13, 2020

@slang25 yeah I was working on this before, the problem there is that the original data are not available at that point and that why the original PR had the solution that we have now that is splitting the path on . which was reverted for a bit, and then causing the PR build to fail, because no test were matched, because the matching part had the full path to the test and not just the method name. But since in F# the method name is more flexible we are seeing this issue. So yes you are right. Hopefully this is not across public api that would need to change. If you need help with the PR feel free the push to your PR branch, it's a draft anyway, and then we can collab on the fix.

@nohwnd nohwnd self-assigned this Mar 13, 2020
@nohwnd
Copy link
Member

nohwnd commented Mar 26, 2020

Fixed in #683

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

No branches or pull requests

3 participants