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

NUnit 3 console should produce xml events for ITestEventListener which contain unique id in the scope of all test agents for NUnit 2 tests #38

Merged
merged 1 commit into from
Aug 24, 2016

Conversation

NikolayPianikov
Copy link
Member

Use the current process id as a prefix for ID

@NikolayPianikov NikolayPianikov changed the title #37 NUnit 3 console should produce xml events for ITestEventListener … NUnit 3 console should produce xml events for ITestEventListener which contain unique id in the scope of all test agents for NUnit 2 tests Aug 22, 2016
@CharliePoole
Copy link
Collaborator

I see two problems with using the process id as the first part of the test id:

  1. Although unlikely, it's theoretically possible for the process id to clash with the id used for an NUnit3 test.
  2. When multiple assemblies are run in the same process, they will all have the same id.

I think this should be fixed by using the same approach as the NUnit 3 driver. Each driver takes on the unique test package ID, stored in the package. This will be unique across all packages and subpackages created for a run. See the nunit3 driver code.

@NikolayPianikov
Copy link
Member Author

NikolayPianikov commented Aug 23, 2016

I've done something like you are talking about. But I am not sure that this code looks fine. I mean that now there is some implicit rule - in what order each method should be called. Also there is another rule - TestPackage.ID should be a string representation of an integer value because it is used as runnerId to call the method RemoteTestRunner.CreateInstance(_testDomain, runnerId). RemoteTestRunner and ProxyTestRunner are located in NUnit.Core.dll. This assembly is used from the "lib" directory as a binary dependency and I think we should not change it. But it works in all general cases

int runnerId;
if (ID == null || !int.TryParse(ID, out runnerId))
{
throw new InvalidSuiteException("ID must be defined at first as a string representation of an integer value.");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is an NUnit V2 Exception, completely unknown to the engine. Let's use NUnitEngineException, which gets special handling by some runners.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, can we add something to the message indicating that it's the V2 driver that requires an int?

@CharliePoole
Copy link
Collaborator

"Implicit rule - in what order each method should be called"

Do you mean that Load must be called first? That has always been the rule. Or is it something else?

"TestPackage.ID should be a string representation of an integer value"

That's true. But the only way to eliminate the problem is to limit NUnit V3 forever to using integer values. NUnit V2 uses ints and we can't change that unless the driver intercepts every event and modifies the id using a lookup table. I don't think we want to do that! So I think we are stuck with this bit of awkwardness so long as we are supporting V2.

@CharliePoole
Copy link
Collaborator

@rprouse This looks good to merge, modulo my comments.

…hich contain unique id in the scope of all test agents for NUnit 2 tests - use current process id as a prefix for ID
@NikolayPianikov
Copy link
Member Author

@CharliePoole Yes I mean Load must be called first

@CharliePoole
Copy link
Collaborator

@NikolayPianikov The DirectRunner takes care that this is done. It probably should be documented somewhere as an assumption that a driver can make. Maybe in the interface comments?

@CharliePoole
Copy link
Collaborator

@rprouse Ready to merge, I think.

@NikolayPianikov
Copy link
Member Author

@CharliePoole comments will never be superfluous

@rprouse
Copy link
Member

rprouse commented Aug 24, 2016

Looks good.

@rprouse rprouse merged commit 74a5e1b into nunit:master Aug 24, 2016
@NikolayPianikov
Copy link
Member Author

@CharliePoole @rprouse

#30 does not work again

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.

3 participants