v5.1 crashes with "Found duplicated test names"

[haf]

 Found duplicated test names, these names are: [ ... ]

--help displays no way to override. I want to run my tests (they are grouped with mutexes rather than names in this case)

[AnthonyLloyd]

Are these blocking you? If not I'll hold off to see if there are any others.

[haf]

Well, it stopped me from upgrading and I froze Expecto with < 5.1, but I can continue to use old versions.

[AnthonyLloyd]

I understand. Its really a breaking change. I'll release tonight.

[forki]

there are two tests. With different names. But expecto complains. What can I do?

[forki]

basically I have

let t1 = testList "list1" [ testCase "a" (fun _ -> ()) ]
let t2 = testList "list2" [ testCase "b" (fun _ -> ()) ]

let all = testList "all" [ t1; t2 ]

all
|> runTestsWithArgs defaultConfig args

[forki]

this should not fail.

[AnthonyLloyd]

O no. I'll look at it today/tomorrow. You can use the allow_duplicate_names config to turn it off for now in 5.1.1.

[forki]

scratch that. I fucked up. It works as expecto-ed.

[AnthonyLloyd]

Cool! I'm stupid also, I forgot I added a test to check exactly this.

Closing...

[Alxandr]

This patch fix was a breaking change that makes the TestSdk integration no longer work. Updating the configuration object will cause anyone who creates their own configuration instead of just using the defaultConfig to break.

[haf]

Perhaps we should introduce an interface that the record implements? After all, keeping binary compatibility would be a very important thing to anything depending on this project.

[Alxandr]

Or hide it's impelementation; only exposing factory/accessors/wither functions. Maybe even lenses could work. Normally, adding properties to an object isn't a problem; but the issue with records is that doing so also adds new parameters to the constructor. We could go with something like:

let myConfig =
     defaultConfig
  |> Optics.set Config.fsCheckMaxTests 200
  |> Optics.set Config.fsCheckStartSize 2
  |> Optics.set Config.mySpiritIsWeak true

It's obviously much more verbose, but still; it would make it much easier to keep binary compatibility. If you make an interface, people are going to implement it... Also, you have the same issue whenever you want to extend the interface. By making the type opaque, this issue goes away. Whenever you add a new property, you also need to make a new lense, which is just a new static property on a class.

[AnthonyLloyd]

Can't we do what FAKE and other projects do and just change things we know about off defaultConfig?

{ defaultConfig with
        Configuration = configuration
        Framework = framework
        Project = project
    }

[Alxandr]

I don't think that works actually (though not sure). Cause it still compiles it down to invoking either the constructor, or pre-defined "with" methods as far as I know.

See doStuff method here: https://sharplab.io/#v2:DYLgZgzgNAJiDUAfALgTwA4FMAEBhA9gHZjYC82A3gLABQ22hAjCNhMgE4CWhA5rfYQBMLbsmwBfftlrBMYmJjABDAK7BkBYmUoNG2gERh8+fQG4Gg7YwAME2jLnYY+AMrIVYEgAoAlNqmyYgC2qJok5BROiqrqYdgA7pzIABYW2gAslpJ02CFhQA===

It compiles to new Conf(defaultConf.n1, newN2). If you break the constructor, this will still fail.

[AnthonyLloyd]

I guess adding to the config is currently a breaking change and needs a major version.

It looks like we really need to resolve the developer API.

We have #204. There is also adamchester/expecto-adapter#37 which I think needs to be considered at the same time.