#30 Error message "File type is not supported" when uses .nunit configuration file - ensure that TestPackage is expanded before usage

[NikolayPianikov]

No description provided.

[CharliePoole]

Please use a standard-format comment or description to tie the PR back to the original issue. Please don't put the original issue number in the title of the PR. Please describe what and why you are doing.

I guess you probably feel the code is self-explanatory, but it really isn't. A project file like .nunit can only occur on the command line. That's why MasterTestRunner expands projects. Abstract test runner is the base class for our internal runners, which are never supposed to see an unexpanded project file. Some internal runners may be running in an agent process, where the project service is not even available. That would give rise to a null reference exception.

Can you explain what was failing and why before you made the change?

[NikolayPianikov]

MasterTestRunner.LoadPackage() is not used at all. It have been done in the commit e6c35c8. EnsurePackageIsLoaded(); was moved from several methods of MasterTestRunner into AbstractTestRunner. Thus an expanding of TestPackage does not work. As I understand the main idea AbstractTestRunner is base class which provides several abstract methods for realization and also provides common logic. Expanding of TestPackage could be a part of common logic, but as I realized I was wrong. I've run a bunch of tests for different values of arguments, and this fix works fine, but something wrong for x86 mode

I've added the code expanding the TestPackage into AbstractTestRunner for config files when TestPackage is defined (in set_TestPackage).

[CharliePoole]

I see the problem now that you explain it. The only thing is that the underlying theme of e6c35c8 was to separate the different functions of MasterTestRunner from the internal runners. In principle, internal runners are not even supposed to know that there is such a thing as an nunit project.

Also, because this issue was created before that commit, it can't really be the source of the problem although it presently contributes to it. Even so, it really shows we need to have some acceptance test that runs nunit projects.

The key problem I see with respect to this change is that there are some cases where we can't actually decide which internal runners should be created and what options to use until after we expand the package. That's probably why you are running into difficulty with X86.

I see two possibilities.

1. Expand the packages as needed in the constructor of MasterTestRunner. This seems like the best way if it's possible - that is, if we already have all the necessary info. I think we do, but it has to be checked to be sure.
2. Write some method like "EnsurePackageIsExpanded" and call that everyplace that the master runner calls into _engineRunner. That means sprinkling the calls in five or six places, but it would certainly solve the problem.

BTW, MasterTestRunner.LoadPackage() is used. It is called from MasterTestRunner.Load(), which may be called by the external runner. Typically, a gui would do that to ensure that everything gets loaded.

[CharliePoole]

I think I'll eventually change the names of some of the MasterTestRunner methods that are the same as those of AbstractTestRunner. They lead to a little confusion.

[NikolayPianikov]

I am not sure we should call LoadPackage(); from MasterTestRunner.Load() and from MasterTestRunner.Reload()

[NikolayPianikov]

Last changes "move all logic related to .nunit config files into MasterTestRunner" has fixed all integration tests including the x86 mode

[CharliePoole]

I'm only glancing at this on my phone at the moment and will look more closely in the morning but it looks as if you are defeating the purpose of the --agents setting by calling load result prematurely in MasterTestRunner. You can only see this by running multiple assemblies with agents limited.

[NikolayPianikov]

You are right. I fixed the issue and added tests

[CharliePoole]

Duplicate entry - did you edit manually?

[CharliePoole]

@NikolayPianikov I started the review but it's very confusing to work with. It looks as if you somehow have all of my commits from an unmerged PR that's waiting for review. Those have not been reviewed yet and are waiting some design decisions I've asked to have reviewed. Did you somehow merge in that branch of mine or have I done something myself? My PR isn't related to yours at all, as far as I know.

I think we have to either hold your PR until mine is reviewed and merged or have you do a clean PR that doesn't include those changes.

I have to drive my wife to the train and won't be back for a few hours. I'll check to see how you think we should proceed when I get back.

[NikolayPianikov]

Yes it looks very strange and it is not clear how it happened, I'm sorry, I'm likely to do something wrong, but I've fixed it
Now seems to be it is correct

[CharliePoole]

This looks much cleaner! The remaining problem I see has to do with where MakePackageResult is called. You moved it from the individual runners to MasterTestRunner. I like the way that removes duplication, but I'm not sure it works correctly now.

Here is what I think will happen, depending on what's on the command-line:

Arguments	Outcome
One assembly	No project element, as expected
Multiple assemblies	No project element, as expected
One project	One project element, as expected
Multiple projects	No project elements rather than one per project
Project plus assemblies	No project elements rather than one wrapping just the project assemblies

The problem is that the project element, if any, needs to be added to each top-level sub-project before they are all merged.

That said, this is a rather esoteric use case. I'd be OK with merging this and creating a new issue for the edge cases if @rprouse agrees.

[NikolayPianikov]

@CharliePoole Current CI issues are relevant?
I see System.Runtime.Remoting.RemotingException: Tcp transport error.
and

1. Failed : NUnit.Engine.Runners.Tests.TestEngineRunnerTests(3).Run
   Expected: 28
   But was: 1

I did not change the class MultipleTestProcessRunner. AggregatingTestRunner has changes related to ProjectPackage only.

[rprouse]

@NikolayPianikov since AppVeyor passed and several of the Travis builds passed, I expect it is unrelated and yet another Mono failure. I've restarted the Travis build.

[NikolayPianikov]

@CharliePoole @rprouse should I do something in this PR?

[CharliePoole]

My comment of two days ago remains. The PR will break existing behavior but only in a corner case. I'm willing to help fix it or merge and fix it later. What do you guys want to do?

[NikolayPianikov]

@CharliePoole could we merge, create issue and fix it later? I just want to add PR, fixing nunit/teamcity-event-listener#14, but I would not like to do it while so many integration tests are red

[CharliePoole]

OK with me if @rprouse agrees.

[rprouse]

I am good to merge and create another issue. One more Travis build is passing this time and the failures are unrelated.