exercism / go-test-runner

GNU Affero General Public License v3.0
15 stars 17 forks source link

Fix random test order #49

Closed junedev closed 2 years ago

junedev commented 2 years ago

Fixes #46

The random test order happened because in one of the intermediate steps the tests were stored in a map. In Go, the order in a map is not guaranteed / will be randomized. That is how the test results got mixed up. It was not caught by the test runner tests because there was only one test case present in the test data.

I verified this code fixes the issue by first changing the test case to include multiple test cases https://github.com/exercism/go-test-runner/commit/0209909feccdff7d3392aa52ee6778b6d98c5e77 and then running the respective test runner test a couple of times with this command that includes "turning off" the Go test cache:

watch -d -n 1 go test ./testrunner -run ^TestRunTests_passing$ -count=1

Every now and then the test would fail. Then I tested again with the code that fixes the issue. No test failure anymore.

coveralls commented 2 years ago

Pull Request Test Coverage Report for Build 1309119606


Totals Coverage Status
Change from base Build 1301385671: 0.0%
Covered Lines: 23
Relevant Lines: 29

💛 - Coveralls
junedev commented 2 years ago

Go reports them in the order in which they appear in the test file. That is the order we want to show to the student as it corresponds to the order of tasks. If we sort that order gets mixed up.

jmrunkle commented 2 years ago

Go reports them in the order in which they appear in the test file. That is the order we want to show to the student as it corresponds to the order of tasks. If we sort that order gets mixed up.

I am not certain if that is guaranteed though. It seems like relying on an implementation detail, but if it is pretty stable we can just live with it and move on.

junedev commented 2 years ago

Putting this on hold. I want to change the test case to include subtest so we can be sure this also works with subtests.

junedev commented 2 years ago

My intuition was correct, my current code does work correctly with the sub tests. I will provide a proper fix as soon as I can.

junedev commented 2 years ago

I went back to the original code and added a test case that includes subtests as well as multiple test cases this time 5a6d515799970ab7de824a1357ad8f4914ddda38. As before, the test was flaky. I used the output the original code generated (in one of the cases where the result was correct) as expected value. https://github.com/exercism/go-test-runner/pull/49/files#diff-861b5d68d4f1de4363b076af08ec6e48f71b7153cd66472b1019665bb5837b0b
Instead of using an Example function, I made it a normal Test now and out the JSON into a file because it got really crowded when I just pasted it into the Example Output.

Then I wrote the proper fix that passes the new test case and isn't flaky (same order of entries in the JSON everytime).