-
-
Notifications
You must be signed in to change notification settings - Fork 96
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
Show fscheck num tests and distribution on test pass #514
base: main
Are you sure you want to change the base?
Conversation
I feel unsure about using pass exceptions. If we do go that route, I think it'd be better to give TestResult.Passed an optional message rather than have two union cases for passed tests. Multiple pass cases feels like it'll lead to easy mistakes. The same apply for multiple pass printers: It'd be better to have one pass printer with an optional message parameter A notable con to some PassException is that we wouldn't be able to hide it, and then it would be available for other assertions. The message might add temptation to use the PassException even when execution should not be shortcutted What are some alternative solutions?
Some questions that might help navigate these choices:
|
I really only need to see the distribution as a debug tool, I only need to see the distribution in the console during development, so using the logger if possible sounds good. I'm not sure of the best way to handle the logger. Currently my tests are creating the Expecto logger at startup and passing it down through all the tests, wrapped in the logger API required by the app code. I updated the PR with a solution that would work for my use case, which is for the property test config to include a logger field, and if that is set it is used when tests pass, but not tied to this |
Such a logger on FsCheckConfig breaks Information Hiding. Do you need this diagnostic information often? |
Ideally every property test would show at least the number of tests that passed, so that's frequent but more of a "nice to have". Test distribution is less frequently needed. How about adding the Or if it's not desirable to expose FsCheck's type directly, the minimum I would want is the number of tests |
I like this line of thought. I think you're on to a good idea. Adding TestResult to the callback would make sense, but we're haunted by the fact that FsCheckConfig actually lives in the core Expecto library. The core library would have to add a reference to FsCheck to return the TestResult. For now, we probably need to look at returning our own data structure. Ideally we want minimal maintenance to keep that data in sync with what's available from FsCheck. I notice TestData is available on every TestResult case. TestData would cover your usecase without having to recreate the whole TestResult type. That would leave us with something like this type TestDataProxy = {
Labels: Set<string>
NumberOfShrinks: int
NumberOfTests: int
Stamps: seq<int * list<string>>
} It would be good to know if the test passed, failed, or was exhausted, but TestData might be good enough to see if people want more. If so, it might be time to bite the bullet and figure out how to move FsCheckConfig to the Expecto.FsCheck package. Thoughts? |
Seems good to me, let me know when you get a chance to look at the changes. I can also add to the readme a recommendation of something like: let testLogger = Expecto.Logging.Log.create "MyTests"
let numTests i =
if i = 1 then "1 test" else sprintf "%i tests" i
let stampsToString s =
let entry (p, xs) =
sprintf "%A%s %s" p "%" (String.concat ", " xs)
match Seq.map entry s |> Seq.toList with
| [] -> ""
| [ x ] -> sprintf " (%s)\n" x
| xs -> sprintf "%s\n" (String.concat "\n" xs)
let defaultFsCheckConfig =
{ FsCheckConfig.defaultConfig with
finishedTest =
fun _ _ testData ->
async {
testLogger.log
Logging.LogLevel.Info
(Logging.Message.eventX
$"Passed {numTests testData.NumberOfTests}.\n{stampsToString testData.Stamps}")
|> Async.RunSynchronously
} }
let myTest = propertyTestWithConfig defaultFsCheckConfig "My test" ... Or we could add this as an alternative base config, taking the logger as an argument? |
I think this one is almost ready, there's still some noise left over from all the revisions: QuietOnSuccess, MaxRejected, readme text related to those two plus replay |
Previously for passing fscheck properties, the number of tests passed wouldn't be shown, and there was no option to show the test case distribution (e.g. percentage trivial tests, etc)
Now
quietOnSuccess
config can be set tofalse
to show this info. NewmaxRejected
config is also acceptedMore generally, tests are enabled to show any message on pass via the
PassWithMessage
exception. This is pretty hacky so this is more of a proof of concept. The alternative was changing the core test types to optionally return a message, which seemed like too big of a change to make on my own.