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

Explicitness of tests is ignored in the presence of a filter with adding multiple negated categories #21

Closed
aeyaey opened this issue Nov 27, 2017 · 10 comments · Fixed by #48
Assignees
Milestone

Comments

@aeyaey
Copy link

aeyaey commented Nov 27, 2017

When using a filter like the one mentioned below, tests marked with ExplicitAttribute (but without any of the mentioned categories) is selected. This is unexpected.
When using a filter with only a single negated category, no ExplicitAttribute marked tests is selected - as expected.

When using the nunit3 framework the filter works a expected.

--where "cat != Db and cat != UnitTest"

resulting in:

<filter>
  <and>
    <not>
      <cat>Db</cat>
    </not>
    <not>
      <cat>UnitTest</cat>
    </not>
  </and>
</filter>

There is a rather hackish workaround, which doesn't work with Nunit3: --where "cat != Db, UnitTest" - but I rather not use that.

@CharliePoole
Copy link
Contributor

It's not surprising that this bug is present because we had to fix it in NUnit 3, so it was probably present in NUnit V2. The V2 framework driver actually uses the NUnit V2 assemblies so it would naturally have the same problem.

Workaround might be to use --where "!(cat == Db || cat == UnitTest)". No guarantees there, however.

Two ways to fix this if we want to...

  1. The adapter could theoretically detect the filter structure (e.g. two negations under an "and") and create a new, equivalent structure that works. Pretty hacky, but necessary if the V2 assemblies are not changed.

  2. Modify the V2 assemblies, perhaps including their source in the driver itself. I'm interested in exploring this idea.

What do other @nunit/framework-team members think?

@jnm2
Copy link

jnm2 commented Nov 27, 2017

If someone is willing to contribute a fix, I'd prefer the fix to be in the correct place (2).

@aeyaey
Copy link
Author

aeyaey commented Nov 27, 2017

The workaround @CharliePoole proposes seems to work. Thanks a lot.

I'm not totally sure what you mean with V2 assemblies. But when excluding multiple categories in Nunit 2.6.4, we use the /exclude command line option:
/exclude:"Db,UnitTest", and that worked fine too.

@CharliePoole
Copy link
Contributor

Sorry to confuse... What I'm saying is this...

The filter code in V3 comes from V2. It had an error we had to fix, but of course, it was never fixed in the original (V2) code. The error is not triggered by use of the /Exclude parameter because it actually produces the filter structure I suggested as a workaround... !(a or b) rather than !a and !b. In fact, in V2 it's very hard to produce the bug, because there is no syntax to trigger it. However it's present in the underlying filter.

@mikkelbu
Copy link
Member

I actually could live with option (1), since I don't think that many people are going to need this, and I think that "including their source in the driver itself" sounds complicated for such an issue (@CharliePoole do you have any idea of how large/difficult the fix is, if we go for solution (2)?)

@CharliePoole
Copy link
Contributor

@mikkelbu For (1) I said "theoretically" meaning "I have no idea how to do it." 😃

For (2) my working assumption is that we can port changes we made to the same code in NUnit 3 back to NUnit 2, but I haven't checked on that. I do know that the NUnit 3 filter code started out the same as the V2 code and that it now works correctly.

@CharliePoole
Copy link
Contributor

@ChrisMaddock Re: pre-release review

I think this may just have been fixed in V3. Perhaps the fix could be ported to V2. If so, then it's a problem for NUnitV2, not for this extension. One of us should review.

@ChrisMaddock
Copy link
Member

ChrisMaddock commented Oct 5, 2019

@CharliePoole This isn't something I'm able to pick up personally, I'm afraid. Given it's a 2 year old issue, I'd say it should be left out of this release, and left for any "future willing maintainer" ;-)

@CharliePoole
Copy link
Contributor

This needs to be re-confirmed because of changes in the underlying code base over time.

@CharliePoole
Copy link
Contributor

🎉 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
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants