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

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

Merged
merged 1 commit into from
Sep 8, 2016

Conversation

NikolayPianikov
Copy link
Member

No description provided.

@NikolayPianikov NikolayPianikov changed the title #30 Error message "File type is not supported" when uses .nunit configuration file - ensure thar TestPackage is expanded before usage #30 Error message "File type is not supported" when uses .nunit configuration file - ensure that TestPackage is expanded before usage Sep 5, 2016
@CharliePoole
Copy link
Collaborator

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
Copy link
Member Author

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
Copy link
Collaborator

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
Copy link
Collaborator

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
Copy link
Member Author

NikolayPianikov commented Sep 6, 2016

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

@NikolayPianikov
Copy link
Member Author

NikolayPianikov commented Sep 6, 2016

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

@CharliePoole
Copy link
Collaborator

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
Copy link
Member Author

You are right. I fixed the issue and added tests

@@ -23,6 +23,7 @@
<DefineConstants>TRACE;DEBUG;NUNIT_CONSOLE</DefineConstants>
<ErrorReport>prompt</ErrorReport>
<WarningLevel>4</WarningLevel>
<NoWarn>1685</NoWarn>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate entry - did you edit manually?

@CharliePoole
Copy link
Collaborator

@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.

…uration file - move all logic related to .nunit config files into MasterTestRunner, ensure that TestPackage is expanded before usage
@NikolayPianikov
Copy link
Member Author

NikolayPianikov commented Sep 6, 2016

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
Copy link
Collaborator

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
Copy link
Member Author

@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
Copy link
Member

rprouse commented Sep 7, 2016

@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
Copy link
Member Author

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

@CharliePoole
Copy link
Collaborator

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
Copy link
Member Author

NikolayPianikov commented Sep 8, 2016

@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
Copy link
Collaborator

OK with me if @rprouse agrees.

@rprouse
Copy link
Member

rprouse commented Sep 8, 2016

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants