Requiring test methods to be public?

[RikkiGibson]

I recently found that it's permitted for test methods to be internal.

using Xunit;

public class TestClass
{
    [Fact]
    internal void TestMethod() { Assert.True(false); }
}

I was surprised by this since most declarations that Xunit is aware of are supposed to be public, such as custom Fact attributes, fields containing theory data, etc.

Was there a specific reason for this? Maybe compat? Is there any consideration toward changing this in the next major version?

cc @jaredpar

[bartelink]

Classes must not be private for technical reasons, methods definitely do not. This is a long standing intentional feature and you can bet anything you want on that not changing

Also be considerate we atting busy people in random discussions like this is something to avoid. Just because someone helped you once and you trust their input does not make that a considerate thing to do, just as you woudlnt randomly send them an email to ask a question you could ask on stackoverflow.

[bartelink]

Ha, thanks for that critical context @333fred - I should dig deeper into profiles before replying :D
In the context of Roslyn, I'd venture the fact that NUnit also runs private Test Methods makes it all pretty moot

Is there a potential win for requiring public methods?

I'd say the main impact in making xUnit require public in 3.x is that anyone doing upgrades from1/ 2.x will need to go searching for the absence of a public on test methods to make sure they continue to run (and let bound tests in F# types (as opposed to modules will need to become members))

[DustinCampbell]

FWIW @RikkiGibson, it's important to allow internal test methods so that they can declare parameters of internal types when using theories and inline data.

[RikkiGibson]

That's very interesting. Thanks! It makes me wonder why, then, MemberData needs to refer to a public member. Since it seems like that locks you out of using your internal types with the generic TheoryData types. https://xunit.net/xunit.analyzers/rules/xUnit1016

What I had in mind initially was the user perspective of: why weren't my tests running? Oh, the class needs to be public, well, the methods are still private, does that work? Oh, the class thing is just due to a reflection performance thing? Why should I have to know that? Why not just say that as an external assembly, Xunit can see things that are public in your assembly?

There's also an xunit analyzer which warns when a public method on a test class is not a test method. So people may see that warning and incorrectly infer that ah, public methods should be test methods, so test methods must be public methods. That is just a case of human folly, though :)

[jaredpar]

That is definitely the trap that I fell into here. It lead to me adding the bug in our infra assuming test methods must be public.

@RikkiGibson thanks for actually verifying the behavior instead of making assumptions like I did 😄

[bradwilson]

The answer to "why must something be public?" is almost always "the reflection cost is too high otherwise", either because of platform performance issues and/or because it causes us to look at too many things which takes too long. Generally speaking, reflecting over the entire world is very bad; it's bad enough that we have to do it just with public types. 😁

As for "but why is X allowed to be non-public?" the answer is usually "because someone made a compelling argument why it should be". In the case of test methods, it's for teams which are shipping test endpoints embedded into their production code that they don't otherwise want to be public, often using our runner as a way to do things like self-verification suites on installed software.

The MemberData one... nobody's come up with a compelling enough reason yet. (I'm not sure anybody's ever tried, if I'm being honest!)