search

exercism / elm-test-runner

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

Extracted record accessor have an extra `.` #34

Closed jiegillet closed 2 years ago

jiegillet commented 2 years ago

Here is an snippet of the test_code.json running on robot-simulator (but I see this everywhere)

  {
    "name": "coordinates",
    "testCode": "Robot anyBearing {x = -1, y = 1} |> ..coordinates\n |> Expect.equal {x = -1, y = 1}"
  }

For some reason there is an extra dot in ..coordinates. No idea where it's from.

mpizenberg commented 2 years ago

@ceddlyburge when you have time to check this one can you have a look?

ceddlyburge commented 2 years ago

This happens at this line of code, so I think it must be a problem with elm-syntax. https://github.com/exercism/elm-test-runner/blob/121f9be14f09954a948680ab05d3feb1e9189769/extract-test-code/src/ExtractTestCode.elm#L118

I'll create an issue / check if it has been fixed. I think there is a new version either out now or coming soon as well, so we might want to wait for that ..

ceddlyburge commented 2 years ago

I think this bug was fixed in this commit: https://github.com/stil4m/elm-syntax/commit/b7e66d73bd8b144de3d05ceb256f16d60a12244b So we should be able to update to the latest version of elm-syntax to get the fix

ceddlyburge commented 2 years ago

@jiegillet : can you add this to your currently open pull request and see if it works? Thanks, Cedd

jiegillet commented 2 years ago

First of all: nice, this version solves the issue!

Second: OMG, this lead me to find a huge issue! @mpizenberg @ceddlyburge

In my PR, I changed the code to group merge the tests and the code snippets. However, whenever there are duplicate names, the jq script merges those. robot-simulator has 20 tests, but the final result only has 4 because most of the tests are named "bearing" or "coordinates". This can't be fixed by a better jq script since there is no way to know which test goes with which snippet. I can think of 2 things to do:

  1. Require all tests to have unique names in the Elm repo
  2. Change both elm-test-rs and extract-test-code so that the name becomes "description 1 > Description 2 > ... > Name" or something like that. These should be unique too of course.

What do you think? We can't merge my PR before we fix that.

mpizenberg commented 2 years ago

Oh damn, I remember we discussed with @ceddlyburge about the fact that we didn't need the ExerciseName in the full name of the tests ExerciseName > test description so we decided to keep only the final description. But I didn't realize at that time that there was tests with more than 1 level in descriptions hierarchies. I suppose instead of keeping just the last part of the test description, we could keep everything after the ExerciseName and we have a guarantee that this will be unique.

This also means that when there are task groups we will have a leading number in the description like 1 > subtest 1a which is not too bad. That's just a reminder of the grouping made by the UI and it doesn't take much characters. And if we were to remove also the task ids from descriptions, we could not guarantee that the name are unique.

jiegillet commented 2 years ago

I checked quickly:

There are some duplicate names in:

No issues with in concept exercises.

Not too hard to fix by hand quickly if we need to.

jiegillet commented 2 years ago

This also means that when there are task groups we will have a leading number in the description like 1 > subtest 1a which is not too bad. That's just a reminder of the grouping made by the UI and it doesn't take much characters. And if we were to remove also the task ids from descriptions, we could not guarantee that the name are unique.

We could also trim the names after we merge the files if we want.

mpizenberg commented 2 years ago

We could, though better have the names being guaranteed unique even for exercism

mpizenberg commented 2 years ago

This means I'll need another version of elm-test-rs. There are ongoing things with elm-test-rs, so I have to check and it might take me a week. I suppose we are not in any hurry here and can wait a week.

mpizenberg commented 2 years ago

Hum maybe this can go faster. I'll let you guys know.

ceddlyburge commented 2 years ago

I think waiting a week is ok ...

mpizenberg commented 2 years ago

Hum, all tests are already required to have unique names. Otherwise the test runner crashes

On Tue, Apr 5, 2022, 08:32 Jie @.***> wrote:

First of all: nice, this version solves the issue!

Second: OMG, this lead me to find a huge issue! @mpizenberg https://github.com/mpizenberg @ceddlyburge https://github.com/ceddlyburge

In my PR, I changed the code to group merge the tests and the code snippets. However, whenever there are duplicate names, the jq script merges those. robot-simulator has 20 tests, but the final result only has 4 because most of the tests are named "bearing" or "coordinates". This can't be fixed by a better jq script since there is no way to know which test goes with which snippet. I can think of 2 things to do:

  1. Require all tests to have unique names in the Elm repo
  2. Change both elm-test-rs and extract-test-code so that the name becomes "description 1 > Description 2 > ... > Name" or something like that. These should be unique too of course.

What do you think? We can't merge my PR before we fix that.

— Reply to this email directly, view it on GitHub https://github.com/exercism/elm-test-runner/issues/34#issuecomment-1088318303, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAWFOCKNY5C6BXLPVVH2NWDVDPNAZANCNFSM5RW54KWA . You are receiving this because you were mentioned.Message ID: @.***>