haf / expecto

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

Improve support for interactive experiences #482

Open farlee2121 opened 7 months ago

farlee2121 commented 7 months ago

Motivation

Expecto is uniquely suitable for interactive environments (like FSI or Polyglot Notebooks) because it treats tests as values. Tests can be defined and executed all in the same code.

However, none of the current runTests methods print output to interactive environments like FSI.

If this PR is merged

This PR adds a new method that collects logs from the test run then returns them as a string. This allows easily displaying test output in interactive environments.

For example, here's a polyglot notebook using runTestsWithCLIArgs

image

Here's the same interactive cell using runTestsReturnLogs image

The new runTestsReturnLogs should obey options the same as other run methods image

ratsclub commented 7 months ago

I will review this later today!

ratsclub commented 7 months ago

One question, instead of expanding the API with a function that only changes the logging output, wouldn't it be better to add an option that tells where the output should go?

farlee2121 commented 7 months ago

In short, I don't think we can generally redirect output for interactive environments. So, configuration creates a more complex experience in exchange for leveraging the existing methods.

It'd look something like

let output = StringBuilder()
runTestsWithCLIArgs [CLIArguments.OutputWriter (fun outputSegment -> output.Append(outputSegment) |> ignore)] [||] tests
output

To me, it seems unlikely we'll want any of the reflection-based runTests variants in interactive mode, but the complexity of the method call can make a big difference in experience. Especially in an environment like FSI where you may have to type it over and over.

ratsclub commented 7 months ago

Hmm, I was thinking of introducing a new case to CLIArguments along the lines of LogOutput of LogOutput and have it be handled inside runTestsWithCLIArgsAndCancel, this way your function runTestsReturnLogs would only pass a default configuration to LogOutput.

I mean, your code looks good to me, but I'm not convinced we should have a function for something that should be a configuration option and be documented on some sort of FAQ.

farlee2121 commented 7 months ago

Suppose we have a call like runTestsWithCLIArgsAndCancel [LogOutput someLogOutput] [||] tests. What is the someLogOutput doing? I don't think there's a consistent place we can redirect the log output for different environments, which means we either need to provide a variety of LogOutputs for different environments or the user defines their own (which kills the simplicity of an interactive experience).

Do you have a more concrete vision of how LogOutput would work?

ratsclub commented 7 months ago

What is the someLogOutput doing?

I was considering something like this:

let runTestsForInteractive cliArgs args =
  let cliArgs = (LogOutput <some-cool-output>)::cliArgs

  runTestsWithCLIArgsAndCancel cliArgs args

This way we can control how the log output is handled inside runTestsWithCLIArgsAndCancel.

farlee2121 commented 7 months ago

The some-cool-output was the part I'm unclear about.

In a polyglot notebook we could do something like LogOutput (fun logs -> logs.Display()). But that won't work other places.

If we accumulate in a string builder, then we're asking users to use at least 3 lines to display output, like demonstrated previously.

ratsclub commented 7 months ago

Oh, I ran an interactive session like on your screenshots and now I see what you mean. The notebook expects an explicit return value to output something, this is why we need to return a string itself... Well, in this case, I think this code does what is needed!

I'm still not comfortable expanding the API with this, but you perhaps found people that use this a lot. If it's useful for them, let's add it.

farlee2121 commented 7 months ago

Hmm, I can't claim high demand for interactive features. It's come up a few times (#365, #471) and it's something I periodically want to do.

If we really don't want to add a method, I could pivot to the option like you suggested, then use it create my own 3rd-party package with the new method.

ratsclub commented 7 months ago

If we really don't want to add a method, I could pivot to the option like you suggested, then use it create my own 3rd-party package with the new method.

I don't want to block your work here, if you feel that we should add this, let's merge it!

I just feel that a third-party is a better way to deal with it, indeed. It just doesn't feel correct to introduce a new function on the library's API that is responsible to change something that should be a configuration.

farlee2121 commented 7 months ago

I guess I'll think about it.

farlee2121 commented 7 months ago

I added some configuration that would allow the interactive experience to be factored into an external library: CLIArguments.LoggingConfigFactory. It has to be a factory, or else there's no sensible way to get the configured verbosity.

I'm a bit concerned that this configuration looks like it's per-call but really it has to lean on Logging.Global.initialize. Thus, all calls within a shared process will share the same LoggingConfig.

As for the interactive run endpoint, I do think it still belongs in the core library. Our own readme talks about running in interactive environments (here and here)