Expose a clear developer API to help extensions such as test adapters

[AnthonyLloyd]

Please suggest parts of Expecto you would like to see exposed.

• [ ] Allow logger to be set in the config. (@Alxandr)
• [ ] duplicatedNames checking (or even better, common exception when it's discovered) (@Alxandr)
• [ ] A function that allows easy filtering of tests based on flatnames. This can be a predicate factory or something similar. (@Alxandr)

[Alxandr]

@MNie

[Alxandr]

@AnthonyLloyd could you make a checklist in the issue description that you append needed things as they are discovered in this thread? And I'll start out with repeating what I said in the other thread?

• Allow logger to be set in the config.
• duplicatedNames checking (or even better, common exception when it's discovered)
• A function that allows easy filtering of tests based on flatnames. This can be a predicate factory or something similar.

[MNie]

I will take a look later today/tomorrow at actual adapter if there is some duplication of logic from Expecto.

Edit: I don't see any duplication of code in vstestadapter.

[Alxandr]

@AnthonyLloyd you might also actually want to create issues per each of the points (and link to them in the list), and let this be more of a planning/overview issue. That way someone could work on bite-sized PRs :)

[AnthonyLloyd]

adamchester/expecto-adapter#37

One question I have is would an addition to the config be considered a breaking change for normal Expecto users?

If not I'd suggest an SDK without an Expecto dependency which uses the test executables CLI. Then backward compatibility would be easier to manage, and also the target framework issues for dotnet core can more easily be solved.

[Alxandr]

I would say config is part of the public api, thus it's considered a breaking change.

[<EntryPoint>]
let main args =
  runTestsWithArgs defaultConfig args tests

This is in the first sample on the readme, and it clearly passes in a config object.

[AnthonyLloyd]

It would only break if the user had created the config without using the defaultConfig and they updated the Expecto version themselves. I don't think there is the same dependency tree version problem.

[Alxandr]

It would break off they in any way modified the default config. If this is what you are after, you should deprecate any api that accepts a config object imho

[AnthonyLloyd]

No, I don't think so.

You compile and include Expecto with your executable. If you upgraded Expecto AND you didn't use { defaultConfig with ... } you would have a build error for additions. I can't think of a way that Expecto gets switched out under you for a new version (apart from Test.Adapter currently).

[Alxandr]

The question would then be; does semver dictate that you should be able to switch out the libraries pre- or post- compilation? Cause if it's the later, then this is a breaking change according to semver. You are forcing a re-compile to use your new version basically.

At this point, I can't say for certain what is "correct". We should probably get some more voices on the issue.

[AnthonyLloyd]

Agreed. Its bending the rules but I can't see how it would bite at this point.

[Alxandr]

It does mean that anything that has a dependency on expecto will break. For instance, if you create a MyOrg.TestCommon which creates the expecto config, it breaks.

[haf]

We should not change this Record without first changing all usages to an Interface.

[AnthonyLloyd]

How does that work? I can't quite picture it.

[haf]

type Config =
  abstract parallel: bool
  abstract parallelWorkers: int
  abstract stress: TimeSpan option
  // etc

type internal ExpectoConfig =
    { parallel : bool
      parallelWorkers : int
      stress : TimeSpan option }
  static member ofIF (c: Config) =
    { parallel = c.parallel
      parallelWorkers = c.parallelWorkers
      stress = c.stress }
  interface Config with
    member x.parallel = x.parallel
    member x.parallelWorkers = x.parallelWorkers
    member x.stress = x.stress

/// This function runs all the tests in the assembly, while allowing you to override
/// configuration via parameters.
let runTestsInAssembly (c: Config) (argv: string[]) =
  let cfg' = updateConfig (ExpectoConfig.ofIF c)
  let tests = discoverAll cfg'
  runTests cfg' tests
  // also, any public function takes the interface.

[AnthonyLloyd]

Won't that still break in the case of adding a field?

[Alxandr]

Yes. The only way to make it non-breaking is to make the properties mutable I would think (which you definitely don't want for a "default" config). Or make it opaque with explicit setters (or as suggested earlier, lenses):

type ExpectoConfig =
  internal
    { isParallel: bool
      parallelWorkers: int }

  member x.Parallel = x.isParallel
  member x.ParallelWorkers = x.parallelWorkers

  member x.WithParallel v = { x with isParallel = v }
  member x.WithParallelWorkers v = { x with parallelWorkers = v }

The issue isn't the added property. It's the modified constructor.

[AnthonyLloyd]

Or make it a DU?

[Alxandr]

Yep. Not entirely sure how that would look though.

[AnthonyLloyd]

type ExpectoConfig =
    | IsParallel of bool
    | ParallelWorkers of int

and the API accepts a ExpectoConfig list

the internal config would be constructed with the last matching in the list or a default.

[AnthonyLloyd]

We actually have this for the CLI arguments:

https://github.com/haf/expecto/blob/master/Expecto/Expecto.fs#L1443

[haf]

The only way to make it non-breaking is...

IAFAIK adding interface members is non-breaking. The Record is internal. @AnthonyLloyd @Alxandr

[AnthonyLloyd]

What happens when a member is added and isn't implemented in the current code?

[haf]

True; you can get MissingMethodException; but rebinding works as long as that method is not invoked AFAIK. It's not the same as the record that breaks completely when you add a field. That said; perhaps the DU-case variant you suggested is the better alternative.

[AnthonyLloyd]

@adamchester @haf @Alxandr Can I ask if you think it could make sense to use the CLI for the Adapter SDK? The reason for thinking of doing this is to dodge all the issues with platform and other config selection that xunit now has. We could pick up the test exes/netcoreapps and run them with test list requests and filtered runs etc. An SDK around doing this could help.

[haf]

You could automate the CLI by allowing for a JSON input-output, command/event per line?

[Alxandr]

This is basically what the MS Test SDK is supposed to give you though. A standardised way of doing test-discoverey and execution, including configuration (using XML of all things, I believe) etc. That being said, running the tests out of proc can have both advantages and disadvantages. In that regard, expecto is kind of weird in that it has both test creation and execution in the same library as opposed to splitting them out.

Regardless, there are a few things you want to think about:

1. Discovery - ability to get a list of tests from an assembly
2. Exception Stack Traces - exceptions needs to be captured in full
3. Debugging - Full debugging with breakpoints etc. I immagine this can get tricky when starting our own process, but VSTest might have mechanisms for it.
4. Output - stdout/stderr should be capturable (prefferably per test, but I immagine that might be difficult when running tests in parallel)
5. Exceptional results - If we run out of proc, we need to be able to deal with the external proc dying for any reason, and signal a failed test execution.

These are just some of the points to consider. Basically, the issue is hard, and some propper planning is probably a good idea if we want to go down such a route. It might also be a good idea to contact people working on MSTest to figure out stuff like debugging and get recommendations.

On the flip side, one big advantage of moving to an external proc, is that we can completely get rid of reflection. We could also require expecto assemblies to have an entrypoint ala

[<EntryPoint>]
let main = runTests

This would circumvent the issues in F# where you need to explicitly create an entrypoint just for the sake of it when creating netcore expecto tests. This entry point might also be possible to add from a nuget package, so that the user wouldn't have to add it themselves for every test library, which would be optimal. Everything from test discovery to test execution would then just be a matter of calling the resulting executable with the correct arguments.

Then again, this is how MSTest works. Mostly. It starts a "test server", and then it sends requests/commands as JSON payloads over socket I believe.

[AnthonyLloyd]

@MNie I forgot to add you in to this question, sorry.

@Alxandr thank this is great. I'm hoping Expecto isn't only weird but we can leverage this position and have a simpler solution than xunit etc for VS Integration.

The biggest issue I see from your list is debugging. I have no knowledge on how to ensure this works. The others I could see an Expecto.SDK package providing easily.

How do we get a handle on that? Maybe @Krzysztof-Cieslak and @forki have ideas too?

[Alxandr]

I would like to say that debugging is vitally important, and while forcing people to choose between running their tests in VS test runner or getting debugging (through running the test assembly directly) is a very typically F# thing to do, it is also a very good way to make people not want to adapt.

[AnthonyLloyd]

Sure.

I have some time to work on this. I'm willing to build the Expecto.SDK package in the Expecto repo that doesn't depend on Expecto but can resolve the test executables, list tests, run lists of tests and return typed results (or a command line that would for debug) etc.

If we could work out what is required for debug and someone build the VSTest adapter side for this.

[Alxandr]

As said; I think the best way forward if we can get debugging out of proc, is to modify the runTestsWithArgs (or whatever it's called) so that it can do everything required by the SDK. That way, the SDK would just be a really thin helper to executing the test assembly (it would take an option object and convert it into cmd line args, and that's about it).

But first, I would investigate debugging, cause if that falls flat, we might need to rethink things.

[MNie]

@Alxandr @AnthonyLloyd @adamchester Maybe its not related exactly to Expecto.Sdk, but we should also think about merging Expecto.TestAdapter and Expecto.TestSdk. Since we shouldn't have situation that we have 2 test adapters, first 1 is commonly used in .net framework related solutions, and the second one in .net core. What do you think guys?

[adamchester]

@mnie sounds reasonable to me

[Alxandr]

Yeah, I don't mind either. I just had a need for testing using .NET CLI, and the existing adapter didn't support it, so I buildt my own, purely for my own sake. If someone wants to take it off my hands, or merge the other one into mine I'm fine with it.

[AnthonyLloyd]

Closing based on more specific plan in #283