Use framework prefilter for massive perf improvement when running fra… #553
Conversation
…ction of tests Implements creation of a prefilter only when VS is passing in a list of test cases. As OsirisTerje is already working on prefiltering with the TfsTestFilter, I didn't work on implementing anything else with preflighting in that method. VS seems to pass in a list of test cases when you are running a single test, which does not pass in a TfsTestFilter, and therefore, shouldn't affect his work.
| if (end > 0) | ||
| prefilters.Add(testCase.FullyQualifiedName.Substring(0, end)); | ||
| else | ||
| prefilters.Add(testCase.FullyQualifiedName); |
There was a problem hiding this comment.
@OsirisTerje Can you think of any edge cases for this?
There was a problem hiding this comment.
I took this code from the NUnitLite code as I was looking for how it did prefiltering. Based on my (very limited) understanding of how VS is working here, it should always match what NUnit generates? But I'm not 100% sure either.
|
@OsirisTerje how do you think this should be tested? |
|
@MatthewBeardmore @jnm2 Sorry for responding late here. What we do today is to use a name filter. When we add the prefilter for the LOAD, it is then given that the results will be equal? Referring back to the other discussion, this kind of filter, - shouldn't it be part of the engine? It would be nice to have a repro solution that we A) measure todays speed and number of tests as a baseline. And then using the new adapter verify B) new adapter speed and C) That number of tests are identical to A). I think this repro should have 1) a lot of tests (we have generated tests in the order of 10000*N earlier) 2) a variety of tests, using different ways of defining them, e.g. test, testcase and testcasesource. |
|
@MatthewBeardmore Can you fix the conflicts ? Would like to get this into the 3.13 release this month. |
|
@OsirisTerje |
|
@Dreamescaper Very good if you can continue on this. You will have to make your own PR though. I have also some concerns about the changes here adding more parameters to several methods, but haven't looked closely into it. Also the introduction of "null" parameters doesn't feel all that good. It might have to be like that of course, but a second look would not harm :-) |
|
I'll take a closer look, and will investigate if existing filtering coverage should be extended. I'll clarify, though.
Are we talking about some manual checklist, or covered as integration tests? |
|
We have some acceptance tests, see cake acceptance tests script and the separate project for that, but it doesn't cover all cases, so yes, we still have to do manual tests. |
# Conflicts: # src/NUnitTestAdapter/NUnit3TestExecutor.cs # src/NUnitTestAdapter/NUnitTestAdapter.cs
|
Sorry, I've been busy for months... I've fixed up the merge conflicts. Let me know if there's anything else that you need. I realize this isn't comprehensive testing by any means, but I've been using this change internally since I created this branch and haven't run into any issues at this point. |
|
@MatthewBeardmore The prerelease is now available at Install-Package NUnit3TestAdapter -Version 3.15.0-dev-01118 -Source https://www.myget.org/F/nunit/api/v3/index.json . Can you check this out? I'm planning on releasing this separately next week, but will like some more checks on it. This issue and PR will then be the only one in that release. @Dreamescaper If you have the possibility to check it out too, it would be awesome. |
|
Sure, I'm adding it today. |
|
@MatthewBeardmore Any news ? |
|
I haven't seen any issues with it so far. |
|
Cool, thanks! I'll get it released within a few days. |
Related to #529
Implements creation of a prefilter only when VS is passing in a list of test cases. As OsirisTerje is already working on prefiltering with the TfsTestFilter, I didn't work on implementing anything else with preflighting in that method. VS seems to pass in a list of test cases when you are running a single test, which does not pass in a TfsTestFilter, and therefore, shouldn't affect his work.