Missing value attribute in test parameters setting causes NullReferenceException in console

[rprouse]

@jnm2 commented on Mon Aug 28 2017

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

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 commented on Mon Aug 28 2017

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 commented on Mon Aug 28 2017

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 #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 commented on Mon Aug 28 2017

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 Write TestParametersDictionary to xml result file in readable format (#2298) #2382
2. Rework Write TestParametersDictionary to xml result file in readable format (#2298) #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 Write TestParametersDictionary to xml result file in readable format (#2298) #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 commented on Mon Aug 28 2017

2 sounds good to me too.

Is it worth investigation #2386 before doing a hotfix? That also sounds potentially critical.

---

@rprouse commented on Mon Aug 28 2017

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

---

@rprouse commented on Mon Aug 28 2017

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.