haf / expecto

A smooth testing lib for F#. APIs made for humans! Strong testing methodologies for everyone!
Apache License 2.0
673 stars 95 forks source link

Expect.equal throws an NRE on failure #330

Closed isaacabraham closed 5 years ago

isaacabraham commented 5 years ago

On Expecto 8.10.1, the following code in an F# script fails with an NRE rather than a "pretty" error.

open Expecto
Expect.equal 1 2 "Doh!"

with the error:

System.NullReferenceException: Object reference not set to an instance of an object.
   at Expecto.Expect.highlightAllGreen(FSharpList`1 diffs, String s) in C:\Users\Anthony Lloyd\src\expecto\Expecto\Expect.fs:line 45
   at Expecto.Expect.printVerses[a,b](String firstName, a first, String secondName, b second) in C:\Users\Anthony Lloyd\src\expecto\Expecto\Expect.fs:line 66
   at Expecto.Expect.equal$cont@317[a](a actual, a expected, String message, Object e, Object a, Unit unitVar) in C:\Users\Anthony Lloyd\src\expecto\Expecto\Expect.fs:line 335
   at <StartupCode$FSI_0014>.$FSI_0014.main@()
Stopped due to error

Is this something I'm doing "wrong" - can you not call Expect. functions directly in a script or something?

isaacabraham commented 5 years ago

Seems that this error goes away if you call runTests on some arbitrary set of tests first, which suggests that some shared static state first needs to be set before you can run Expect. functions. Is there a way to avoid this? It's surprising to couple a test runner to individual predicate / test helpers.

AnthonyLloyd commented 5 years ago

It's a colours config that is used in the error messages. I've defaulted to 0 if no config sets it. Will go out soon. Workaround is to runTests on an empty set of tests before any Expect.

AnthonyLloyd commented 5 years ago

@isaacabraham can I just check what you are looking to use this for. Just want to consider the default colours. If it's in another test runner I'm not sure if colours would be useful.

isaacabraham commented 5 years ago

@AnthonyLloyd Yes, I just opened the REPL and wanted to experiment with the testing library behaviours outside the context of the test runner.

isaacabraham commented 5 years ago

@AnthonyLloyd additionally, when running in the context of e.g. azure dev ops, a load of control codes seem to be emitted in the console and get in the way of the real textual output. However, that's a side issue related to colours rather than this NRE.

isaacabraham commented 5 years ago

I suspect that the commit linked above (5a675cb) won't address the root cause e.g. someone running a script on mac or linux will still get this error.

AnthonyLloyd commented 5 years ago

@isaacabraham Need to set --colours 0. They are ANSI control codes. No sensible way of detecting if they are not supported and we default to 8 colours.

AnthonyLloyd commented 5 years ago

I suspect that the commit linked above (5a675cb) won't address the root cause e.g. someone running a script on mac or linux will still get this error.

They'll get the ANSI colour codes.

If you know of any library that handles colour well please let me know. Note we moved from Console commands to ANSI to make it atomic and stop the bug often seen with colour being incorrectly reset.

haf commented 5 years ago

@AnthonyLloyd What would you have against checking whether the terminal is interative? https://github.com/haf/expecto/issues/315#issuecomment-480893080 — if you have a terminal like Travis CI that supports ANSI escape characters, they would be correctly parsed, or if you have Azure, then presumable tty would be false. Would perhaps @isaacabraham be willing to run a script on Azure to test whether this assumption is true?

isaacabraham commented 5 years ago

I think we're conflating the issue here. That was simply about getting an NRE if you call Expecto functions outside of the test runner. Putting a check for Windows won't help that at all I would imagine.

But of course happy to test with dev ops. But sounds like the color argument above will do the trick - I'll try that out

AnthonyLloyd commented 5 years ago

@haf Nothing against it if we can be sure it's a solution. Just wary of complexity that's hard to test.

@isaacabraham sorry, cross issues colours discussion.

haf commented 5 years ago

Putting a check for Windows won't help that at all I would imagine.

No, that's separate; the change that fixed your problem was to default the colours setting to something other than null (None).