Nunit-console: ColorConsoleWriter.WriteLabel causes NullReferenceException

[jnm2]

I've gotten this 7 times out of the last 9 builds on the build server at work. The only thing that should have changed from last night is that there's a lot more parallelization going on. Oh, also I updated to NUnit Framework 3.8.0, duh...

c# Run Settings DisposeRunners: True WorkDirectory: [redacted] TestParametersDictionary: System.NullReferenceException: Object reference not set to an instance of an object. at NUnit.ConsoleRunner.ColorConsoleWriter.WriteLabel(String label, Object option, ColorStyle valueStyle) at NUnit.ConsoleRunner.ColorConsoleWriter.WriteLabelLine(String label, Object option, ColorStyle valueStyle) at NUnit.ConsoleRunner.ResultReporter.WriteRunSettingsReport() at NUnit.ConsoleRunner.ResultReporter.ReportResults() at NUnit.ConsoleRunner.ConsoleRunner.RunTests(TestPackage package, TestFilter filter) at NUnit.ConsoleRunner.Program.Main(String[] args)

Fails with error -100.

[jnm2]

This looks like the only line that could possibly be causing the NRE: https://github.com/nunit/nunit-console/blob/master/src/NUnitConsole/nunit3-console/ColorConsoleWriter.cs#L111. We should either write nothing or add an ArgumentNullException check to start with.

[jnm2]

And here is the root problem: https://github.com/nunit/nunit-console/blob/master/src/NUnitConsole/nunit3-console/ResultReporter.cs#L95 needs to be updated in concert with https://github.com/nunit/nunit/pull/2382. We have a compatibility issue now that none of us thought of. Due to NUnit Console's NRE, NUnit Framework 3.8.0 is incompatible with NUnit Console 3.7.0 and earlier. Even without the NRE, there's no fallback so it would look like there are no test parameters and that may be a compat concern in itself.

Can we tag this issue integration test wishlist? ... 😟

/cc @ChrisMaddock @rprouse @CharliePoole

[rprouse]

Well, damn. @CharliePoole was worried that removing the value attribute and moving it into the items might cause problems for transforms. None of us thought to check our own code and our own tests don't cause this to happen.

I think this warrants an emergency hotfix which I can do tonight. I think we have two options,

1. Back out #2382
2. Rework #2382 so that it is backwards compatible by including the dictionary settings as a single string and also include the individual elements for updated runners.
3. Rework #2382 removing the child elements and just add the value.

My preference is for the second option. I can make the changes starting in about an hour.

@jnm2 will you be available this evening for code reviews?

[ChrisMaddock]

2 sounds good to me too.

Is it worth investigation https://github.com/nunit/nunit/issues/2386 before doing a hotfix? That also sounds potentially critical.

[rprouse]

@ChrisMaddock I agree, nunit/nunit#2386 also looks serious, we should track both. I will comment there.

[rprouse]

I am also going to copy this issue to the framework repo for the backwards compatible fix. We should also address this in the next release of the framework to display the child elements if available, so I will leave this open in this repo.

[jnm2]

I prefer option 2 as well.

will you be available this evening for code reviews?

Yes. I cleared the evening. If there's anything else you want me to help with let me know. Otherwise I'll be working on the remoting replacement till I hear from you.

[jnm2]

NUnitLite may have the same NRE issue as NUnit Console.

[jnm2]

Yep https://github.com/nunit/nunit/blob/master/src/NUnitFramework/nunitlite/ColorConsoleWriter.cs#L111

[jnm2]

I renamed https://github.com/nunit/nunit/issues/2390 so that we can start a new framework issue with a name similar to this issue but for NUnitLite without being too confusing hopefully.

[ChrisMaddock]

@rprouse - did you make a start on this in the end? I'd like to get it in for the next console release - I can take it if you prefer? 🙂

[rprouse]

Go ahead and take it.

On Dec 12, 2017 7:55 AM, "Chris Maddock" [e-mail] wrote:

@rprouse https://github.com/rprouse - did you make a start on this in
the end? I'd like to get it in for the next console release - I can take it
if you prefer? 🙂

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/nunit/nunit-console/issues/285#issuecomment-351043192,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAeJBKyEGk7OwDzMLRxCv4x9-0xeNLS9ks5s_nepgaJpZM4PE9ba
.

[mikkelbu]

As stated in #339 I can also take a look at this, but actually I'm unsure what this issue is about 😄 .

From https://github.com/nunit/nunit-console/issues/285#issuecomment-325472312 I have _"We should also address this in the next release of the framework to display the child elements if available, so I will leave this open in this repo."_.
Is the task to use the individual item elements if they are available and not the value attribute as it is the case now? (i.e. some special handling for settings with the name TestParametersDictionary)

If I run with some parameters the result looks fine, but I guess that it is only because of #2391.

Run Settings
    DisposeRunners: True
    WorkDirectory: C:\src\NUnit\nunit-console\bin\Release
    TestParametersDictionary: [run, yes], [finish, no]
    TestParameters: run=yes;finish=no
    ImageRuntimeVersion: 4.0.30319
...

[CharliePoole]

This was marked as critical in august. The original meaning of critical was that we would fix the thing immediately and put out a hot fix. I think that's what Rob intended and I agree with him.

The criticality is the NRE. Everything else is non-critical and should have been discussed as another issue rather than holding this one up. My suggestion... Fix the NRE and leave everything else.

It doesn't serve us to mix multiple levels of urgency in the same issue unless we can really get it all done in the time frame needed by the highest level. For critical, the guideline in the past was 24 to 48 hours.

[ChrisMaddock]

I reduced the priority to 'high'.

We instead put a backwards compatibility fix into the 3.8.1 version of the framework - so this issue was only prevent for users using the --params tag with version 3.8.0, which was vLatest for under 24 hours. The fix we put in was to add a value attribute to the top level tag of structured settings.

@mikkelbu - thanks for the offer, but I have now this mostly done on https://github.com/nunit/nunit-console/compare/issue-285 - just need to finish wrapping a test around it. So long as Rob doesn't plan on bringing the 26th date forward, I should be able to get it finished off. (However, if you've already written the code, feel free to push your version instead - save me finishing off that test! 😄)

Is the task to use the individual item elements if they are available and not the value attribute as it is the case now...but I guess that it is only because of #2391.

Yep - exactly that. 😄

[mikkelbu]

@ChrisMaddock I've only written the "production-code" (and performed manual testing), so I also need to implement some tests, so you can keep this issue 😄 .

[ChrisMaddock]

Sorry - I should have come back to update the issue that I'd made a start!

[CharliePoole]

@ChrisMaddock Fixing the critical part and then reducing the priority is a simple solution to having mixed urgency problems on the same issue. Good thinking!