Test setup failure no longer aborts test run

[seancorfield]

Describe the bug
It looks like the integration of pluggable test runners has broken some of the assumptions around project test fixtures:

Running test setup for the publisher project: worldsingles.application.fixtures/pre-test

Test setup failed: Syntax error macroexpanding at (worldsingles/user.clj:1:1).
Running test setup for the system project: ws.system.fixtures/pre-test
...

If the "Test setup failed", the entire test run should abort. It used to abort, and it no longer does.

To Reproduce
Add a project-specific test setup function that throws an exception (for a project that will be among the first to be tested).

Run poly test in a way that runs tests for multiple projects, including that one.

Expected behavior
The test setup for that project will fail (b/c it throws an exception) and it should abort the test run.

[seancorfield]

@imrekoszo I haven't looked at the PR in detail that introduced this but I suspect it catches and reports an exception around test running, but no longer propagates it to the caller?

[seancorfield]

Also: need to ensure that if test setup succeeds, then test teardown must be run even if the tests fail.

[tengstrand]

Add tests, see comment in Slack:

• If test setup fails, the whole test run should stop (and test teardown should not happen)
• If test setup succeeds and the tests pass, the test teardown should run and if it fails, the whole test run should stop
• If test setup succeeds but the tests themselves fails, the test teardown should still happen -- and if it succeeds, behave according to the test failures
• If test setup succeeds but the tests themselves fails, the test teardown should still happen -- but if that fails, the whole test run should stop

[imrekoszo]

Thank you @seancorfield and apologies for the breakage. Could you please share the version (sha) of poly you are using to make sure we're looking at the correct one?

Also, does reverting poly to 28cca05514500fe995d4107dd5649c162b57b02a or earlier fix this?

Here's how it's done in 28cca005 (just before pluggable TRs):

polylith/components/test-runner/src/polylith/clj/core/test_runner/core.clj

Lines 121 to 125 in 28cca05

	(when (execute-fn setup-fn "setup" name class-loader color-mode)
	(try
	(run-test-statements name class-loader test-statements run-message is-verbose color-mode)
	(finally
	(execute-fn teardown-fn "teardown" name class-loader color-mode))))))))

And after pluggable TRs:
https://github.com/polyfy/polylith/blob/master/components/test-runner-orchestrator/src/polylith/clj/core/test_runner_orchestrator/core.clj#L67-L71

and
https://github.com/polyfy/polylith/blob/master/components/test-runner-orchestrator/src/polylith/clj/core/test_runner_orchestrator/core.clj#L107-L117

[imrekoszo]

@seancorfield @tengstrand I created a repro case for this at https://github.com/imrekoszo/polylith-kaocha/tree/poly-issue-234-repro

Please check it using bb test :all :project at the following commits:

1. before pluggable test runners: imrekoszo/polylith-kaocha@ef97176

#$ ~/opensource/polylith-kaocha ‹ef97176› 
; bb poly version                             
  0.2.14-alpha (2022-03-30)

2. with pluggable test runners: imrekoszo/polylith-kaocha@1227633

#$ ~/opensource/polylith-kaocha ‹1227633› 
; bb poly version
  0.2.15-alpha-pr196-06 (2022-04-28)

To me the observable behaviour appears identical which makes me believe this had not worked before pluggable test runners either.

[seancorfield]

This was the original issue I raised where it did not behave correctly -- and this was fixed and working as expected before pluggable test runners: #161 -- I verified that it worked as expected after that fix.

I will create a failing repro at work and wind back through the commits to see if it was something other than the pluggable test runner work that broke that behavior.

[imrekoszo]

I'm really looking forward to the result of your bisection. As far as I could tell, 403dccc is what fixed #161 and there haven't been significant changes to the test runner code between that and 28cca05, which is the last commit before pluggable: 403dccc...28cca05

28cca05 has this bug according to my tests.

[imrekoszo]

Something just occurred to me - @seancorfield did you verify that fix with multiple projects where setup fails for a project that isn't the last one being tested? Since #161 the project whose setup failed won't have their tests run but I'm not seeing any code that would skip running tests for the other projects in that case.

[seancorfield]

FWIW, this bug was recently (re?) fixed by #232 so I'm closing this (I just happened to trip over this exact situation again for the first time since I opened this issue!).

[tengstrand]

I have added these tests to the issue-234 branch:

polylith/scripts/create-example.sh

Lines 403 to 422 in e0fd83a

	cd $examples/for-test
	echo "current-dir=$(pwd)"
	set +e
	echo "### 50/55 examples/for-test, issue 208 - Mix clj and cljc source directories ###"
	poly test :all project:okay color-mode:none > $output/for-test/mix-clj-and-cljc.txt
	echo "exit code: $?" >> $output/for-test/mix-clj-and-cljc.txt
	echo "### 51/55 examples/for-test, issue 234 - Test setup fails ###"
	poly test :all project:okay:setup-fails:x-okay color-mode:none > $output/for-test/setup-fails-stops-entire-test-run.txt
	echo "exit code: $?" >> $output/for-test/setup-fails-stops-entire-test-run.txt
	echo "### 52/55 examples/for-test, issue 234 - Test teardown fails ###"
	poly test :all project:okay:teardown-fails:x-okay color-mode:none > $output/for-test/teardown-fails-stops-entire-test-run.txt
	echo "exit code: $?" >> $output/for-test/teardown-fails-stops-entire-test-run.txt
	echo "### 53/55 examples/for-test, issue 234 - Test failed ###"
	poly test :all project:failing-test:okay color-mode:none > $output/for-test/failing-test-runs-teardown-and-stops-entire-test-run.txt
	echo "exit code: $?" >> $output/for-test/failing-test-runs-teardown-and-stops-entire-test-run.txt
	echo "### 54/55 examples/for-test, issue 234 - Test failed, teardown failed ###"
	poly test :all project:failing-test-teardown-fails:okay color-mode:none > $output/for-test/failing-test-and-teardown-fails-stops-entire-test-run.txt
	echo "exit code: $?" >> $output/for-test/failing-test-and-teardown-fails-stops-entire-test-run.txt
	set -e

The tests:

1. mix-clj-and-cljc.txt - works correctly (runs without errors and returns 0).
2. setup-fails-stops-entire-test-run.txt - works correctly (stops the entire test run and returns 1).
3. teardown-fails-stops-entire-test-run.txt - doesn't work correctly (continues running the tests for the next project, x-okay, and returns 0).
4. failing-test-runs-teardown-and-stops-entire-test-run.txt - works correctly I think (stops the entire test run, executes the teardown code, and returns 1).
5. failing-test-and-teardown-fails-stops-entire-test-run.txt - works correctly (we have a failing test and an exception in the teardown code, which stops the entire test run).

Kaocha only stops the execution if the --fail-fast parameter is passed in (point 4). I haven't verified the Kaocha test runner though (e.g. that it returs 1 when a test fails).

I'm not sure which default behaviour people want here when a test fails. I'm personally fine with stopping all test execution and returning 1 immediately, but I want input from the community. Maybe this should be configurable, like in Kaocha?

@seancorfield @imrekoszo @furkan3ayraktar

[seancorfield]

The reason that failed setup stopping the test run is important is because it may not cause tests to fail (for a variety of reasons) and so that setup failure will scroll past and, unless you just happen to see it, you have no idea there's a problem -- that's what happened for us and it meant that a dependency-related bug, not detected by check, got past CI and onto our staging environment. That's why 2. is so important to me.

I'm not as worried about teardown failure stopping the entire test run so I don't really care about 3.

I'm not quite sure what you're describing in 4. and 5. -- do you mean that a test failure somewhere in a project causes the test run to stop after completion of all tests in that project (which I think is the right behavior) or do you mean that a test failure causes the project's test run to stop immediately and no further tests are run in that project? (which seems over-aggressive to me: you generally get more information by letting the test run for that project complete).

[tengstrand]

I agree that 2 is important and that we should keep it like that.
Even if 3 is not as important, it seems resonable that it should also stop the entire test run.
Number 4 works like this right now: if you run tests for e.g. project A and B, and if a test fails in A, then the entire test run stops (no more tests are executed for either A or B). I think this is how it has always worked.

If we want to support that a project continues running its tests after a test failure, then we need to be able to configure this (stop directly, or finish all tests for current project). I'm fine with not introducing configuration for this, and instead keep it as it is today and stop the entire test run (not executing any more test for current or subsequent projects).

I'm of course interested in what you have to say about it, but I talked with Furkan and he is also fine with stopping the entire test run if a test fails.

[imrekoszo]

IMO 2 and 3 are the most important ones and central to the current issue. I agree with Joakim that both of these should stop the test run. As teardown fails in 5 I think that also belongs here.

WRT 4 I think that's a different topic. Usually, and especially in CI I want my test suite to run in full then have a report with all the failing tests. This is default for kaocha (only talking about kaocha itself, not my polylith-kaocha connector). Less often, I will go "okay what else do I need to fix" and in these cases I'll pass --fail-fast (to kaocha) so it gets me to the next failing test. Whether a feature like this would be useful for poly or not, and what should that feature's details look like is outside the scope of this ticket I think. Adding a test to verify the current behavior could be useful though.

[seancorfield]

I looked back over some of our CI failures and it looks like when a test fails, the remaining tests in that namespace are still run but then the test run stops -- which I think is what @tengstrand is describing -- and I'm fine with that behavior, even though there is definitely value in continuing on through all the namespaces in a given project as an alternate behavior (the default behavior of clojure.test on its own is to run all tests in all namespaces in a project and then report the total failed/passed, even if tests in some namespaces failed in the middle).

[tengstrand]

Please have a look at this PR @imrekoszo @seancorfield @furkan3ayraktar.

With this fix, if we run the tests for project A and B and a test or test setup/teardown fails for project A, then test for project B will not be executed (which they are today).

[imrekoszo]

@tengstrand I added a comment on your PR, it looks good to me otherwise.

[tengstrand]

Thanks @imrekoszo and @seancorfield for the feedback!
This is now merged to master.
I plan to create a release soon with everything that currently is in master.