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

Improve support for interactive experiences #482

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

farlee2121
Copy link
Collaborator

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

@farlee2121 farlee2121 requested a review from haf November 6, 2023 17:26
@@ -879,7 +879,7 @@ module internal ANSIOutputWriter =
override __.WriteLine() = write "\n"

let mutable internal colours = None
let internal setColourLevel c = if colours.IsNone then colours <- Some c
let internal setColourLevel c = colours <- Some c
Copy link
Collaborator Author

@farlee2121 farlee2121 Nov 6, 2023

Choose a reason for hiding this comment

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

I couldn't discern a reason this needed to be settable only once, and it prevents the colour setting from working as expected in interactive environments

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@@ -643,3 +643,44 @@ module Tests =
/// Returns 0 if all tests passed, otherwise 1
let runTestsInAssemblyWithCLIArgs cliArgs args =
runTestsInAssemblyWithCLIArgsAndCancel CancellationToken.None cliArgs args

let runTestsReturnLogs cliArgs args tests =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could use feedback on this method name.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Maybe runTestsForInteractive

Copy link
Collaborator

Choose a reason for hiding this comment

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

What do you think about runTestsInteractively?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I thought of a similar name, but I feel like it denotes the test run itself is interactive, as in there will be decision points for the user during the test run.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Well, runTestsForInteractive seems reasonable to me, then.

Seq.fold (fun s a -> foldCLIArgumentToConfig a s)
ExpectoConfig.defaultConfig cliArgs

match ExpectoConfig.fillFromArgs config args with
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I believe this results in double args parsing, but it seemed like the least complex option (and easiest to follow) and we're not doing that much parsing generally. It also only applies to this interactive run method

@ratsclub
Copy link
Collaborator

I will review this later today!

Accepts the same arguments as `runTestsWithCLIArgs`, but returns the console output as a string.

This is useful for interactive environments like [F# interactive](https://learn.microsoft.com/en-us/dotnet/fsharp/tools/fsharp-interactive/) and [Polyglot Notebooks](https://marketplace.visualstudio.com/items?itemName=ms-dotnettools.dotnet-interactive-vscode), where console output is not available but the returned string can be displayed instead.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I plan to add a screen shot once I can show the proper package reference.
Something like #r "nuget: Expecto, 10.2.0".

@ratsclub
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

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
Copy link
Collaborator

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
Copy link
Collaborator Author

I guess I'll think about it.

@farlee2121
Copy link
Collaborator Author

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)

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.

2 participants