exercism / elm-test-runner

GNU Affero General Public License v3.0
3 stars 5 forks source link

Issue when the there's no test output written #3

Closed ccare closed 4 years ago

ccare commented 4 years ago

Hi,

So this is an interesting case. I invoked the runner on the following code...

module AggregateScorers exposing (..)

aggregateScorers : List String -> List String
aggregateScorers playerNames = List.unique

In this case we get output to (I assume stderr) containing this

-- NAMING ERROR ------------- /tmp/exercism-elm/elm-1-a/src/AggregateScorers.elm
I cannot find a `List.unique` variable:
4| aggregateScorers playerNames = List.unique
                                  ^^^^^^^^^^^
The `List` module does not expose a `unique` variable. These names seem close
though:
    List.minimum
    List.unzip
    List.any
    List.filter
Hint: Read <https://elm-lang.org/0.19.1/imports> to see how `import`
declarations work in Elm.
Compilation failed while attempting to build /tmp/exercism-elm/elm-1-a/tests/Tests.elm

That output should end up in results.json. The test runner should output the compilation error in the top-level message property. (see https://github.com/exercism/automated-tests/blob/master/docs/interface.md#message)

Also, in this particular case, the compilation failure meant that the junit.xml file wasn't written and in this situation the python conversion code explodes (because it's expecting the input file to exist).

I would PR a suggestion, but I'm not sure how you want to solve this. I guess one option could be to capture stderr and the exit status of npm test and then provide these as additional inputs to the python script.

mpizenberg commented 4 years ago

I can confirm that if elm-test crashes because of elm compilation error or other crash it does not create the report file and the message is sent to stderr. It seems to me that the possible error codes are

So we could indeed switch on the error code. Do you know what other tracks are doing in that case? is there an alternative to the python script we are using to convert from junit (and remove one dependency in the docker)

iHiD commented 4 years ago

In that situation, you should use the top-level message key with enough of the error that a user can work out how they fix it.

I've invited you both to the #experiment-1-beta channel on Slack. You can use the instructions to see how the Ruby one handles different scenarios.

mpizenberg commented 4 years ago

I was trying to write something based on those exit codes but it seems npm test transforms that exit code 2 into a 1 so it is not possible to differentiate with the npm test.

As a consequence, in commit 63f2d637 I replaced npm test with

node_modules/elm-test/bin/elm-test --compiler node_modules/.bin/elm ...

Then in the case of an exit code of 1 (a crash) I manually build the message key with the stderr redirected into a temporary error.txt file. It ain't pretty but I believe it works on all situations I've tested. Let us know @ccare if you can confirm.

ccare commented 4 years ago

That looks really cool - I'll have a play and let you know. Thanks!

mpizenberg commented 4 years ago

I suppose this is ok, shall I reset master on this then?

iHiD commented 4 years ago

I think that sounds sensible.

mpizenberg commented 4 years ago

@iHiD master is protected (for good reasons ^^) are you able to force push? Otherwise I'll just merge and remove the 3Mb .elm/ directory afterward. 3Mb in the repository history isn't that problematic.

iHiD commented 4 years ago

I can do, yeah. What am I pushing where?

mpizenberg commented 4 years ago

Basically,

git clone git@github.com:exercism/elm-test-runner.git
cd elm-test-runner
git reset --hard remote-elm-home
git push --force
iHiD commented 4 years ago

Done :)