microsoft / testfx

MSTest framework and adapter
MIT License
714 stars 253 forks source link

MSTest V2 and data driven tests #464

Closed AbhitejJohn closed 2 years ago

AbhitejJohn commented 6 years ago

Description

Filing this to better the experience for MSTestV2's data driven tests in VS IDE. Currently with 1.3.2 version of the Fx/Adapter pair here is how the tests are displayed:

image

Here are the problems that this notation has:

  1. One does not have a way of running/debugging an individual test data. (#171 details this as well)
  2. The summary test result that started showing up with the latest package isn't much of a help since the TE already has a node indicating that one of the tests has failed.

For the first issue, if mstest starts associating each data row with a separate test case object that should start showing tests similar to how nunit/xunit does today which is more granular and consistent with the Test Explorers tree view. Open to suggestions on better ways of achieving this. The second issue is kind of tied to the first. If we must send out a summary result though, is there a way that clients can filter this out? Is there a property that marks these results?

/cc: @jayaranigarg , @pvlakshm , @cltshivash

michael-hawker commented 4 years ago

Just found out about DataTestMethod and DataRow, it's super powerful, but it would be great if the individual data rows appeared underneath the test case.

Basically it'd be great if the test case became a new parent in the tree in order to:

  1. Better navigate the results of individual test scenarios (and be able to jump to the DataRow attribute where it was declared on double-click like any other test)
  2. Providing easy access to the individual scenario's result of expected and actual
  3. Providing a click/UI point to be able to run/debug a specific row

Open Questions: a) Should each DataRow test count towards the number of tests? For consistency in bubbling the total number of test-cases, I think yes. b) Changed the above to have a - separator as keeping the name as DataRow(1,2) seems like it would lead to confusion with the parenthesis being used everywhere else to indicate number of tests.

I.e.

nohwnd commented 4 years ago

@michael-hawker @peterwald Is this someting you should take into consideration in your FQN proposal?

I thought this is already happening. That is, we already produce a test case per datarow, are 1) and 2) "only" matter of presenting it differently?

For 3) I don't think we have a simple way of identifying the attributes on the test? And only run 1 of them? This would probably be a lot of effort to implement in adapters as well.

a) In console this already reports as 2 failed tests. In VS it reports like 1 failed test, but on the detail of the test it actually shows 3 failed (1 for each test case, and 1 for the test itself) which is imho misleading. In trx it is represented as UnitTestResult -> InnerResults -> UnitTestResult and also reports as 3 failed tests. To me this shold report 2 failed tests in all cases

b) I actually used just int param to try it out, and this in fact confused me 😁 So I support that, but imho we should think more about the syntax, for example - 1,1 - 2,1

is also easy to confuse with -1,1.

peterwald commented 4 years ago

I thought this is already happening. That is, we already produce a test case per datarow, are 1) and 2) "only" matter of presenting it differently?

xUnit and NUnit already function this way for dynamic test cases. They produce a single independent TestCase for each "instance" of the test (test + data) and when executed, they return a single TestResult for each of those TestCases. MSTest instead returns a single TestCase for the test, and when executed returns multiple TestResults for that single TestCase. To make matters worse, MSTest also returns a "summary" case as the first item in the list. So, for a dynamic test with 2 DataRows, we receive 3 results as @nohwnd noted.

NOTE: We can't just remove the summary test result at this point, because there are existing tools that depend on it. We would need to fully convert over to a single TestResult per TestCase in order to not break compatibility.

ALSO NOTED: In prior conversations with the testing team, I believe the reason this choice was made originally is because they were trying to support data driven tests with a huge number of data points. For instance, a dynamic test that is driven by a database table with 100,000 records. In that case, discovery would create massive numbers of test cases that would be displayed in the Test Explorer. By only showing the single TestCase, they would make the UI easier to navigate. That being said, the Test Explorer has been updated to support much higher numbers of TestCases since then and it is my belief that this is no longer a significant issue.

For 3) I don't think we have a simple way of identifying the attributes on the test? And only run 1 of them? This would probably be a lot of effort to implement in adapters as well.

I think we should just convert to creating multiple TestCases, as this would be more consistent and also easier to implement.

a) In console this already reports as 2 failed tests. In VS it reports like 1 failed test, but on the detail of the test it actually shows 3 failed (1 for each test case, and 1 for the test itself) which is imho misleading. In trx it is represented as UnitTestResult -> InnerResults -> UnitTestResult and also reports as 3 failed tests. To me this shold report 2 failed tests in all cases

This is due to the "summary" result that is added by the adapter. If we were to change it to be like NUnit/xUnit this issue would go away.

b) I actually used just int param to try it out, and this in fact confused me 😁 So I support that, but imho we should think more about the syntax, for example - 1,1 - 2,1

is also easy to confuse with -1,1.

Most of the others use a parametric form like TestName(a = 2, b = 3) or something similar.

Haplois commented 2 years ago

This is implemented as part of #910.