Test count in start-run report is always 0

[cyanite]

This is because the MasterTestRunner is never "loaded" (i.e. the Load method called) before the tests are run, which then makes CountTests return 0. Curiously, no code ever calls Load except some unit tests, which thus seem to test behavior not actually encountered when running the program.

There is also a bug in Load which makes it crash for aggregated test runs.

I don't know if there is a good reason to not always call Load. It seems to work fine for me on a large-ish .nunit test suite with 1 nunit3 test assembly and 50 nunit2.

I will create a pull request which "fixes" the bug in Load (makes it return null where it otherwise crash; no one uses the result anyway), and adds an option to nunit-console, --preload, to load the MasterTestRunner before running the tests. This fixes the bug.

[mikkelbu]

Some googling in the repository gave #146 and in particular the following #146 (comment) about why the count of test cases for the <start-run> event is currently zero. I've not looked into the code or PR in more detail.

Note: we possibly have the same issue in the netstandard version, src/NUnitEngine/nunit.engine.netstandard/Runners/MasterTestRunner.cs, but perhaps this will be solved by #388.

[cyanite]

Well, that makes sense. However @CharliePoole 's comment:

However, it's only in the console that we limit the number of agents and the console runner doesn't need to have a count in advance.

Is only true by default. For example, there is a teamcity addin shipped with NUnit which produces special output while running tests, and I have a similar addin whose purpose is to provide progress information to a tool that's running nunit.

For our particular test workloads we won't actually end up starting a lot of processes, but I certainly appreciate that it can be a problem in general. My PR adds an command line option.

[cyanite]

Just to be clear on the above: I need the test count in order to display progress.

[CharliePoole]

@cyanite Your point is a good one. If an extension wants a valid count then we should try to give it. A command-line option feels like a hack though. Can we allow an extension to indicate what it needs or directly ask for it instead?

[cyanite]

@CharliePoole I agree it's a bit hack-ish. Having the extension activate the load would be ideal, but I don't see how it's possible at the moment. The extension interfaces don't have a lot of methods. The one I am using is the ITestEventListener with just one method.

Also, the call to Load (in my "hack") is all the way in the console-runner executable, which otherwise just calls Run directly. I feel a command line option, while not ideal, would still be useful. Maybe a longer, less ambiguous name.

Otherwise we perhaps need something like an optional IConfiguration-something interface that extensions can implement.

[rprouse]

@cyanite thanks for the pull request, but I agree with @CharliePoole on this one, a command line option isn't the right way to go, so I closed that PR. An IConfiguration type interface might be the right way to go, but that will take a bit of design, so I am going to label this issue as such.

[cyanite]

@rprouse, alright, then. I'll have to keep the changes in my local build since we need progress reports on our large suites. Also note that the PR fixes another crash issue (in Load).

[cyanite]

@rprouse Of course it would be easy to add a method/property "ShouldLoad" to ITestEventListener but that would break all existing extensions. Alternatively, a new callback-style interface could be optionally implemented, which would have a method where the test runner would be passed. But which test runner, then? Any ITestEventListeners are added at the MasterTestRunner level, but other extension points are added in different places.

[rprouse]

Or we could just provide a way for extensions to get the test count that would ensure that they tests were loaded?

[CharliePoole]

Least intrusive approach is probably declarative. Either a new attribute or a property of Extension attribute could pass info to the engine about what an extension requires. I imagine a flags enum.

[cyanite]

@CharliePoole Yes... although the problem with that is that my extension only activated when a certain environment variable is present, so as not to mess with regular console runs. With an attribute it would activate all the time. Compare to the teamcity plugin which is normally disabled (and enabled somehow, I don't know). Things like that seem to be needed since there is no way (I know of) to enable and disable plugins on the command line, is there?

Alternatively: With a new single-purpose interface, it could decide whether or not to activate dynamically:
interface ITestRunnerConfiguration { void ConfigureRunner(ITestRunner runner); } or something, with whatever names. "Configure" might be a too strong word. In my case, I would just call runner.Load. Maybe "BeforeTestRun" is better. (this alternative is moot if you can indeed enable and disable extensions on the command line, in which case I also vote for the declarative approach).

Edit: An even simpler interface, in line with what @rprouse talked about, ITestCountSomething { void SetCount(int count); }.

Edit edit: If there were a general way to enable/disable extensions on the command line (like the hardcoded one for --teamcity), @CharliePoole's declarative approach would be fine also. Don't we want such a feature? Shouldn't be hard to do, I think.

[CharliePoole]

@cyanite TL;DR I agree with your last suggestion. 😄

Enabling / Disabling extensions is purely an engine feature, which may be accessed by a runner. Conceptually, the extensions themselves are imagined to be always active, if they are called by the engine. Disabling one just means it won't be called.

Of course the engine can't possibly know anything about whether an extension should be disabled, since that's a function of the runner (nunit3-console in this case). However, an extension author can decide whether an extension should be initially disabled or enabled, indicating the choice via ExtensionAttribute. The runner can use whatever logic it likes to change that setting, but whatever it does is completely hidden from the extension. IOW, with the current design, an extension can't do different things based on being enabled or disabled. In fact, that makes no sense in the case of the disabled state since it will never be called. An extension that needs to do some initialization can do so on first call.

If we were to allow an extension to disable itself, it would need to have a way to tell the engine that it was disabled. We could do that through a callback interface to the engine or by having the extension implement an interface to be monitored by the engine. That would allow the logic for enablement / disablement to be placed in the extension as you have done.

A command-line approach is appropriate IME if the user may or may not want to use the extension on a run-by-run basis. That's exactly the case with the teamcity extension: we only want it to be active when the console is being executed by teamcity. Unfortunately, that forces us to put special logic in the console runner to understand the purpose of the teamcity extension and when to enable it. I didn't much like doing that, but there seemed to be no choice at the time.

A more general approach would be to allow the console runner to support extensions as well and to package console and engine extensions together in one assembly. That was the initial design, in fact, but it was never implemented.

I would be in favor of two shorter term fixes to the overall problem:

1. Create a more general console option to enable / disable extensions by their extension path.

2. Provide an optional interface to be implemented by any extension that wants to provide information to the engine, such as "treat me as disabled" or "I need to have a valid count of tests on the start of the run". Note that the last example doesn't try to tell the engine how to get such a count, it might do it in different ways depending on the other command-line settings, especially in case one of those settings asked for the number of active agents to be limited.

[cyanite]

@CharliePoole Great. As for disabling/enabling by path, note that nunit3-console --list-extensions don't show any paths, which would make that a bit hard to discover. I suggest doing it by name instead.

As for the optional interface, this would solve my problems nicely. The problem with an interface is that it's annoying to add methods to it later without having to change the code in all extensions. We could also add an interface with a single method returning a configuration object with a base class. Then you'd only need to recompile the extension in most cases, although that part is unavoidable of course, for an engine extension. Option 1 suggestion:

interface IExtensionConfiguration
{
    bool Enabled { get };
    bool NeedsValidTestCaseCount { get };
}

Or option 2:

interface IExtensionConfiguration
{
    ExtensionConfiguration Configuration { get };
}

public class ExtensionConfiguration
{
    public virtual bool Enabled => true;
    public virtual bool NeedsValidTestCaseCount => false;
}

Something like that?

[CharliePoole]

On second thought, maybe Enabled is confusing. When Enabled is false, currently, the assembly is not only never called but is never loaded at all. Anything returned by an interface, of course, can only be used after loading the assembly. We could call the state of a loaded extension that doesn't want to be called any further "inactive" rather than "disabled". Does that make sense?

I think we may prefer a flags enum for the specified behaviors the extension needs from the engine. That has two advantages:

1. It localizes future changes to the enum itself, not the interface. Of course, we could add methods to an interface without breaking callers that don't use the new methods, but it's a little more risky. Note that this interface would be in the nunit.engine.api assembly, whose assembly version never changes.

2. It allows users to cast an int to the enum type in order to pass special settings, not known to NUnit, to their private runners.

So my preferred interface definition would be something like...

public interface IExtensionRequirements
{
    bool Active { get; }
    ExtensionRequirements { get; }
}

[Flags]
public enum ExtensionRequirements
{
    None = 0,
    NeedsValidStartRunCount = 1
}

In this scenario, the engine would have to operate as follows...

Extension is disabled:
    No change. **The extension is never loaded and never called.**

Extension is enabled:
    IExtensionRequirements implemented:
        Call IExtensionRequirements.Active (causes JIT-loading of the assembly)
        Active?
            Check for and implement any requirements, then call the assembly as needed.
            [This part needs work. What if the requirements have no connection with the type of extension? For example, a framework driver that asks for a valid start-run? Should the requirements be categorized by extension type?]

        Inactive?
            Do not check for requirements, do not call the extension's primary interface.

    IExtensionRequrements not implemented:
        No change. **Extension is called and JIT loaded as needed.**

This is a big change. The bold lines above are what we currently do, the rest would be new. I think we should define the interface and put it in place and implement the Active / Inactive status in the engine in a single PR. Handling the count should be separate, since it has some complications that depend on what options the user has specified.

[CharliePoole]

I think we need input from the @nunit/engine-team about the interface before going ahead.

[CharliePoole]

@cyanite Responding to an earlier comment I missed... the "path" to an extension has nothing to do with the file system. Extension points have a "Path" that describes them uniquely. An extension is implemented by a particular type on a particular extension point. Extension points and extensions are in a many-to-many relationship, so the command-line option would be rather long.

IMO, we need a separate issue for command-line enabling / disabling if we want to do it.

[cyanite]

@CharliePoole, for background, in my extension, I activeate/deactivate it based on the presence of an environment variable. I'd prefer a command line option. I don't think we need to be able to disable/enable at the granularity of extension points (paths); I'd be happy with just disable/enable the entire extensions as a whole. Also because extensions conceivably can hook into multiple extensions points that work together.

So a command line like ... --enable-extension=Batt.NUnit3Addin.BattNUnit3Addin (which is the name listed by --list-extensions) would suit me.

As for the interface/enum, it looks good to me.

[ChrisMaddock]

If this remains the case in the latest version of the engine, let's remove the count attribute from start-run in v4. It's inaccurate, and there's no clear efficient solution to supply this information at this point in time. It can always be added again as a future feature, if someone comes up with a solution.

[CharliePoole]

If you eliminate this, then there's no easy way for a GUI to display a progress bar.

I think we should figure out when and why this changed. It must have been after I forked the engine for the GUI, because the counts I get work for me.

[ChrisMaddock]

Sounds good. I assumed that it had never worked so nobody could be relying on it, but sounds like that's not the case.

[CharliePoole]

The OP had a very specific situation here, involving an extension. We need to run some tests... probably best to add them to the CI in fact. I'll take a look.

[CharliePoole]

I finally looked at this. The Gui doesn't have a problem because it always calls Explore before it's even possible to run and Explore forces a call to Load. But Run is supposed to force a call to Load as well, and I'm out of time right now for looking further for the moment.

UPDATE:
Both GUI and NUnit engines work the same way. Before the run starts, a call is made to CountTestCases passing the filter that will be used for the run. The return from CountTestCases is used in the start-run report. In addition, CountTestCases loads the test in both engines.

Consequence: still need to do more in-depth debugging to understand this. I believe @mikkelbu 's 2018 comment citing my own 2016 comment is correct. I just need to figure out where that happens in the code. Five years is a long time! In any case, I think there's a tradeoff to be made here. We can easily give a correct value if --agents has not been specified. It will, however, cause a performance hit if we load all agent processes twice and could cause strange results if test cases are being generated randomly (or dynamically) at load time.

Regarding MasterTestRunner.Load: It was once used but we now perform the load at a lower level whenever it is needed, using the internal runner or runners created by MasterTestRunner. That's where something is going awry. For the 4.0 engine we will have to figure out whether to change the ITestRunner interface to eliminate Load. Alternatively, it may make the code clearer if we use it, event though it simply delegates to the internal implementations.

[CharliePoole]

See this #146 (comment) for further info about why it is the way it is.

Possibly, we could give a valid count so long as --agents is not specified or is less than the number of assemblies. @cyanite @nunit/engine-team What do you think of that idea?

[CharliePoole]

@nunit/engine-team
As we suspected, this requires a re-design for 4.0. There are two different use cases to deal with...

1. Number of agents is not limited or the limit is greater than or equal to the number of test assemblies.
2. Number of agents is limited and the limit is less than the number of test assemblies.

In case 1, there's no problem. We can easily return a valid number of test cases.

In case 2, we can only count the tests if we either ignore the limit or load and count test cases one at a time. Call those option A and option B.

With option A we violate the number of processes allowed to be active (the --agents limit). We could, however, allow only a subset of those processes to execute at one time. So the limit would mean "max number of test assemblies executing" at one time rather than "max number of active agents".

Option B avoids some problems but creates others. Assume we create a single process and count the tests for each assembly using that process - loading and unloading each assembly and keeping a tally. In this case, we will never exceed the agents limit. However, any load-time initialization like TestCaseSource code will execute twice, which may be a big performance hit for some people.

Alternatives I can see for V4.0...

1. Check to see whether the max agents value is specified and if it's less than the number of assemblies. If not, just count normally. If so, then either...
   1.1 Use option A
   1.2 Use option B
   1.3 Keep returning zero
2. Invent some other way to report progress, with the possibility of percent complete going down as more tests are discovered.
   2.1 Report progress as the number of assemblies left to run.
   2.2 Two-level progress reporting, i.e. number of assemblies and progress within each assembly
   2.3 Any of the above approaches but provided as an extension by runners that need progress.
3. Stop reporting progress entirely.

All of the above are breaking changes.

I'm marking this as blocked until we decide what to do.

[ChrisMaddock]

For v4.0, I think we just remove the attribute as the minimal change for correctness. The feature has never worked, so feels more like we're adding a new feature - and that would be a separate discussion that needs someone willing to implement it.

[CharliePoole]

You suggested that earlier. As I said it's a breaking change for me if it no longer works at all - as opposed to leaving it as it is now. Obviously, that will affect our Compatibility Project.

I agree that it has never worked for the console runner. But if we're talking about the engine, it works depending on choices made in the runner. In the case of the console runner, we made the choice to call Run without first calling Load, which results in a zero count. We kept the attribute because other runners might want to use it, calling the engine differently.

Can we separate the issue of the console from that of the engine? If you don't like any of the other options I listed, can we resolve this by simply documenting how things work?

Regarding "a separate discussion that needs someone willing to implement it"... For some one to pop up and implement a feature, you have to first decide it's a desired feature. IMO that's what discussing the options is about. In addition to the options I listed, there are some others that were previously discussed on this thread. Can we pick a path to follow in the future, even if it's going to be in a post-4.0 release?

[CharliePoole]

I created issue #921, for documenting <start-run>. I think we need to do this in any case.

[ChrisMaddock]

Sorry, I missed that it was just to do with whether load had been called or not.

In that case, does that make this a simple bugfix, that doesn't need to be in 4.0?

[CharliePoole]

I think so. We made it breaking because of what the OP asked us to do about it.

Levels of non-breaking change I can see:

1. Just document better how it works.
   1.1 For runner authors (have to call Load or Explore)
   1.2 For extension authors (whether you get a valid count depends on the runner)
2. Add CountTestCases as another way to indirectly call Load (I was confused by the fact that it doesn't!)