exercism / elm-test-runner

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

Extract test code #21

Closed ceddlyburge closed 3 years ago

ceddlyburge commented 3 years ago

Hi Matthieu, it sounds like the ask is to change the test description / name, to include the string from describe

This is definitely doable, but if you are changing elm-test-rs anyway, I feel like it might make more sense to just use the test name (and require that it be unique, which I think is something elm-test-rs already requires). Adding the "1 >" type text won't look as slick for the students, and I think the exercism test output already has a task_id property for this purpose.

mpizenberg commented 3 years ago

No you're right sorry. Indeed I'll change the name in elm-test-rs to return only the last describe component, just as you do in this extractor so you don't need to change that. What I failed to say was the focus on the JSON output with a name and test_code fields so that we can easily merge both outputs with jq for each matching name field, later in the run.sh script.

mpizenberg commented 3 years ago

Here is how we might merge the two later (after I upgrade elm-test-rs) : https://stackoverflow.com/questions/54956517/use-jq-to-combine-two-arrays-of-objects-on-a-certain-key

mpizenberg commented 3 years ago

I've updated elm-test-rs to v1.1 with the new "task_id" detection and using only the last describe component for "name". So you can now test directly in this PR the integration with the test code extractor.

ceddlyburge commented 3 years ago

Ok, so to get this pull request to a mergeable state, I think I need to change the output, to something like this, right?

[
  {
    "name": "599 pence should be 5.99 pounds",
    "test_code": "... all the stuff here ..."
  },
  {
    "name": "5.99 pounds should be formatted as £5.99",
    "test_code": "... all the stuff here ..."
  },
  ...
]

I don't think this will be a big deal, we already have the name and the test code, its just a case of formatting the output a little differently (and escaping the code).

mpizenberg commented 3 years ago

Yep exactly. No need to do much work for escaping even, it's enough to make it an encoder and then call Json.Encode.encode 2

mpizenberg commented 3 years ago

For a data structure like:

type alias ExtractedTest =
    { name : String
    , testCode : String
    }

encode : ExtractedTest -> Value
encode = Encode.object ...
ceddlyburge commented 3 years ago

HI Matthieu, the code now outputs the results in the required json format. I think there is still some work to do to integrate it and use it with the test runner / elm-test-rd

mpizenberg commented 3 years ago

By the way, the jq magic that merges the extracted code with the tests results might interest @ErikSchierboom and @iHiD as an example for other people in the same situation as ours, that is with the extracted code in a different json file: https://github.com/exercism/elm-test-runner/pull/21/files#diff-00a80c5821edea2ebf676056aa4c9a24e57379ef52cefecb2ccffaaf4cc362c9R68

ceddlyburge commented 3 years ago

One tiny weird thing is the additional empty line after the let for tests using a let ... in.

This comes from the way elm-syntax formats the output. We could probably submit a bug request, or a PR to the repo, they were very prompt last time there was a problem

ceddlyburge commented 3 years ago

Let me know what you think @ceddlyburge and if you see something wrong while testing it with actual example solutions of various shapes.

It all looks good to me, although I haven't tested it. I'm in favour of merging and testing with some example solutions.

mpizenberg commented 3 years ago

Actually there is a problem with the order of tests after the merge. It seems jq command group_by sort elements by the corresponding key ("name" here) and so the order of tests is not preserved :( I'll see if I can find an alternative

ceddlyburge commented 3 years ago

If not maybe we can do something in node ...

mpizenberg commented 3 years ago

Alright, I've fixed the order, then fixed the bug I introduced in the fix, then fixed a forgotten version: 2 in the error case. Finally, I think we are good now.

ceddlyburge commented 3 years ago

Skillz :)

mpizenberg commented 3 years ago

Stackoverflow skilzzzz :)