Fix code extractor to match `elm-test-rs` 2.x

[jiegillet]

This solves #30 and #34. Please check commits for details.

I didn't add tons of tests to test all of the branching, because I think the vast majority would be convoluted and unrealistic code.

This should be merged before #36, because it also introduces the new test name path "describe name 1 > describe name 2 > ... > test name".

[ErikSchierboom]

Are there tests to verify that this doesn't break anything?

[jiegillet]

Heh, all the tests are in the other PR :p It shouldn't change anything for the current setting, but I can run a few sanity checks tomorrow.

[ErikSchierboom]

👍

[mpizenberg]

Thanks @jiegillet could you explain the main changes introduced in the extracting code to handle these situations?

Also, regarding the current limitations, it is nice to better handle test expressions that can start with other things than test "name" <| but it is still valuable to forbid those if possible when creating test files.

For example, in case of usage of let value = ... in (test "name " <| \_ -> -- using value for expectation) if we only extract the part with the expectation from the test, the student will not know what value is equal to when the test is run. This is especially important for concept exercises where students do not have access to the test file and rely on what is displayed on the website interface.

Similarly, any kind of expression that results in unknowns when considering only the part of the code with the expectations (the snippet) can cause some confusion for students if they don't have the whole context in the test snippet.

So I'm wondering if instead of trying to handle those situations, we might want instead to output some keywords that our CI can spot and report to fail before merging such tests with ambiguous snippets extractions.

[mpizenberg]

Considering the comment above, it might be worth splitting this PR in two. (1) the part that updates the test name to be unique by taking into account the description hierarchy. And (2) that improves the snippet extractor capabilities.

(1) is unambiguously good and needed before merging the 2.0.1 elm-test-rs update.

(2) might need some more discussions

[jiegillet]

Considering the comment above, it might be worth splitting this PR in two. (1) the part that updates the test name to be unique by taking into account the description hierarchy. And (2) that improves the snippet extractor capabilities.

Alright. For this PR, (1) and (2) are deeply intertwined, so I reverted to the original version and did the minimal changes to achieve (1). @ErikSchierboom this change will not make any difference to the way tests are currently running, it's only updating the name of the test code snippets, and that doesn't influence the final result at all because the field is actually ignored (it shouldn't be, but that is fixed in the other PR).

For the rest, I do agree that we should have tests that capture as much information as possible in the body of the test, especially for concept exercises. We could have CI checks, but ultimately it seems to me that code review is the most important tool we have. However, as we repeatedly saw with robot-simulator, when review fails (or for old exercises), it is important to have code that is flexible enough to handle imperfect tests instead of crashing.

So in my opinion (in the future), we should

1. Update the elm documentation to be a lot more clear about test expectations/limitations
2. Have code extracting code that goes beyond the expectations anyway
3. Add CI checks (we can run elm-test-runner in the elm CI, maybe with specific checks)

[jiegillet]

@ErikSchierboom could you merge this?

[ErikSchierboom]

Sure!