runFor is very flaky

[olivercoad]

Disclaimer: I haven't actually used runFor (or canopy's build-in test runner much at all, for that matter) and just picked up these bugs while reading the implementation for #190 (comment) so I may have a couple things wrong.

Although it seems neat to implement runFor by duplicating the test suites with added suites to start/switch browsers using Once, it actually leads to a number of bugs.

Does not work with runFailedContextsFirst

When runFailedContextsFirst = true, the test suites are not run in order, so the added test suites which start browsers are not run at the right time. This will lead to no browser being open for all the failed tests.

Does not work if there are any WIP tests

If there are any WIP tests, then all test suites not including a WIP test (including the ones to start/switch browser) are not run.

Silent errors starting browsers

If an error occurs while starting a browser or switching browser, it will be silently ignored.

The browser is started or switched to using suite.Once. If an exception occurs, it will be caught and failSuite will be run with the exception to report the error for all tests in the suite. However, because the suite does not have any tests to report the exception for, it won't even get reported. All the tests intended to run on that browser will just silently get run on the previously open browser instead.

This means you can't rely on testing multiple browsers in CI. For example, if you used

runFor (BrowserStartModes [chrome; firefox])

and firefox failed to open in continuous integration for whatever reason, then all the tests would silently be run in chrome (twice). There would be no indication in the logs that this had occurred - in fact the suite contexts would still read "(firefox) ...".

The current implementation of runFor for easy reference

canopy/src/canopy/runner.fs

Lines 241 to 269 in a593649

	let runFor (browsers: Browsers) =
	// suites are in reverse order and have to be reversed before running the tests
	let currentSuites = suites
	match browsers with
	| BrowserStartModes browsers ->
	let newSuites =
	browsers
	|> List.rev
	|> List.collect (fun browser ->
	let suite = new suite()
	suite.Context <- sprintf "Running tests with %s browser" (toString browser)
	suite.Once <- fun _ -> start browser
	let currentSuites2 = currentSuites |> List.map(fun suite -> suite.Clone())
	currentSuites2 |> List.iter (fun suite -> suite.Context <- sprintf "(%s) %s" (toString browser) suite.Context)
	currentSuites2 @ [suite])
	suites <- newSuites
	| WebDrivers browsers ->
	let newSuites =
	browsers
	|> List.rev
	|> List.collect (fun browser ->
	let suite = new suite()
	suite.Context <- sprintf "Running tests with %s browser" (browser.ToString())
	suite.Once <- fun _ -> switchTo browser
	let currentSuites2 = currentSuites |> List.map(fun suite -> suite.Clone())
	currentSuites2 |> List.iter (fun suite -> suite.Context <- sprintf "(%s) %s" (browser.ToString()) suite.Context)
	currentSuites2 @ [suite])
	suites <- newSuites

Again, I don't actually use runFor myself so feel free to correct me.

A fix

I think it should be simple enough to implement correctly by *essentially copying run () but running all the suites for each browser (L207-232). *with refactoring

[olivercoad]

Will make a PR if you reckon that would work @lefthandedgoat

[lefthandedgoat]

I will have to look into it more and think on it. Im not sure how many people even use it.