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

Exploring tests with filter returns all tests #27

Closed
ivanli opened this issue Feb 14, 2019 · 9 comments · Fixed by #47
Closed

Exploring tests with filter returns all tests #27

ivanli opened this issue Feb 14, 2019 · 9 comments · Fixed by #47
Assignees
Milestone

Comments

@ivanli
Copy link

ivanli commented Feb 14, 2019

NUnit.Engine.ITestRunner.Explore(...) does not obey the filter that is passed as argument to it and returns a list of all the tests within the assembly.

This behaviour is inconsistent with calling NUnit.Engine.ITestRunner.Run(...), which obeys the filter as expected and only executes the tests which pass the filter.

@CharliePoole
Copy link
Member

@ChrisMaddock I'm reviewing each issue in the light of the upcoming release.

This one IMO should get in. One of us may need to complete it if @ivanli doesn't respond on the PR. It sat idle for a long time, so he may be working on other things by now.

Additionally, the PR would need to be re-reviewed in light of the changes made by PR #30.

In any case, I think this is important enough that it should get into the release.

@CharliePoole
Copy link
Member

CharliePoole commented Mar 22, 2020

This was confirmed once but it needs to be re-confirmed because the underlying code has changed quite a bit since the issue was filed.

UPDATE: Confirmed!

@CharliePoole
Copy link
Member

Keeping this as a v2 framework driver bug. Since nunitv2 actually has no Explore function the driver will have to build it on top of the framework by loading the entire assembly and subsequently applying the filter.

In fact, this adds a new feature. The rationale here is

  1. Not having the feature creates a bug because the engine expects it to be present.

  2. Adding the feature in nunitv2 itself makes no sense since it would only ever be used by the adapter.

@CharliePoole
Copy link
Member

@ivanli Can you try one of the packages at https://ci.appveyor.com/project/CharliePoole/nunit-v2-framework-driver/builds/31644230/artifacts to see if it works for you?

@ivanli
Copy link
Author

ivanli commented Apr 5, 2020

Hi Charlie, it's been a while since this issue came up and we've moved to nunit v3. I can dig up the old project and give it a go sometime this week. Will update.

@CharliePoole
Copy link
Member

@ivanli Yes, it has taken a while. I only now took over this project again, so I'm cleaning up old stuff. If you are able to check it out, that will be handy but if you can't do it easily just let me know.

@ivanli
Copy link
Author

ivanli commented Apr 24, 2020

@CharliePoole Apologies for the delay for getting onto this. I've just re-ran the unit tests on our custom test runner and it now works properly. Also re-confirmed that it fails on V3.8.

Thanks very much for the fix! Looking forward to V3.9 so we can update the test runner on our end as well.

@CharliePoole
Copy link
Member

@ivanli @ChrisMaddock @jnm2

This will be sure to confuse someone in the future - maybe even future me! So...

@ivanli did a PR (#28) which needed a bit more work, so I used his branch to create a new one along with PR #35. That one, however, was automatically closed (I think) when I changed the name of 'master' branch to 'main'. So, I started a new PR (#47), which has now finally been merged.

I believe all the needed changes are in it but I didn't want to risk losing all the work so I merged without waiting for any more review. This can be tested by grabbing the appveyor asset or the latest dev build from myget. If more is found to be needed, please let me know here or file a new bug.

@CharliePoole
Copy link
Member

🎉 This issue has been resolved in version 3.9.0 🎉

The release is available on:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment