Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add task ID tags to concept exercise tests #476

Merged
merged 15 commits into from
Mar 28, 2022

Conversation

jiegillet
Copy link
Contributor

@jiegillet jiegillet commented Mar 26, 2022

Linked to this PR.

This PR tags concept exercise tests to their task ID, by grouping them with describe "N" where N is the task number. elm-test-rs v2.0 will use that description to assign tags in the test runner.

Make sure to use the "Hide whitespace" option, otherwise it will impossible to review.
Screen Shot 2022-03-26 at 22 13 16

The PR also updates the version of elm-test-rs to v2.0 to match the test runner and cleans up outdated comments in bin/build.sh.

@jiegillet jiegillet added x:module/concept-exercise Work on Concept Exercises x:size/large Large amount of work labels Mar 26, 2022
@jiegillet
Copy link
Contributor Author

@mpizenberg the CI is failing, I think it's because of mpizenberg/elm-tooling-action, I think it only has elm-test-rs 1.0.0.
What's the best course of action? We can either update elm-tooling-action (but I don't know the ramifications of that) or stay with 1.0 here since it's not strictly necessary.

@mpizenberg
Copy link
Member

mpizenberg commented Mar 26, 2022

@jiegillet You're right, I didn't update elm-tooling-action to use the latest version of the elm-tooling lib, which does support elm-test-rs v2. This was because we had many discussions with @lydell to eventually be able to support any binary within elm-tooling and I was waiting for that before updating elm-tooling-action. But we both got busy on other things.

For us right now, the easiest fix is updating the version of elm-tooling here: https://github.com/mpizenberg/elm-tooling-action/blob/master/package.json#L11
I added both of you Simon and Jeremie to elm-tooling-action maintainers if you want to make a PR there anytime in another branch directly.

@mpizenberg
Copy link
Member

I just published a new v1.3 elm-tooling-action and updated the dependency here to be able to download v2.0 of elm-test-rs.
I didn't check the content of the tasks changes yet though. If @ceddlyburge or @ErikSchierboom could have some time to check task groups that would be awesome. Otherwise it will have to wait till next week for myself (probably end of next week cause I'm piling up work ^^)

@ceddlyburge
Copy link
Contributor

HI @mpizenberg , is your comment meant for exercism/elm-test-runner#36?

@jiegillet
Copy link
Contributor Author

@ceddlyburge (sorry to butt in) no, it was meant for here. I wanted to upgrade to elm-test-rs v2.0 here too (for running bin/build.sh) but the CI couldn't download it from mpizenberg/elm-tooling-action. It has now been updated so that issue is fixed.

The rest of the PR could still use a review :)

@ceddlyburge
Copy link
Contributor

Well, I am slightly confused as to what is going on! But I checked out these two pull requests, and I ran the Bandwagoner exercise from this pull request through the new test runner in the other pull request, and it does set the "task_id" correctly, as can be seen in the results.json copied below.

{
  "version": 3,
  "status": "pass",
  "tests": [
    {
      "name": "has Coach type alias with correct fields in correct order",
      "task_id": 1,
      "status": "pass",
      "message": null,
      "output": null,
      "test_code": "Coach \"Steve Kerr\" True\n |> Expect.equal {name = \"Steve Kerr\", formerPlayer = True}"
    },
    {
      "name": "has Stats type alias with correct fields in correct order",
      "task_id": 1,
      "status": "pass",
      "message": null,
      "output": null,
      "test_code": "Stats 55 27\n |> Expect.equal {wins = 55, losses = 27}"
    },
    {
      "name": "has Team type alias with correct fields in correct order",
      "task_id": 1,
      "status": "pass",
      "message": null,
      "output": null,
      "test_code": "let\n  \n  \n  coach  =\n      Coach \"Red Auerbach\" False\n  \n  \n  stats  =\n      Stats 58 22\n  \n  \n  team  =\n      {name = \"Boston Celtics\", coach = coach, stats = stats}\nin\n  Expect.equal team (Team \"Boston Celtics\" coach stats)"
    },
    {
      "name": "createTeam creates a Team structural type",
      "task_id": 2,
      "status": "pass",
      "message": null,
      "output": null,
      "test_code": "let\n  \n  \n  coach  =\n      Coach \"Red Auerbach\" False\n  \n  \n  stats  =\n      Stats 58 22\n  \n  \n  team  =\n      createTeam \"Boston Celtics\" stats coach\nin\n  Expect.equal team (Team \"Boston Celtics\" coach stats)"
    },
    {
      "name": "can replace coach for a team",
      "task_id": 3,
      "status": "pass",
      "message": null,
      "output": null,
      "test_code": "let\n  \n  \n  coach  =\n      Coach \"Willis Reed\" True\n  \n  \n  newCoach  =\n      Coach \"Red Holzman\" True\n  \n  \n  stats  =\n      Stats 6 8\n  \n  \n  team  =\n      Team \"New York Knicks\" coach stats\n  \n  \n  newTeam  =\n      replaceCoach newCoach team\nin\n  Expect.equal newTeam (Team \"New York Knicks\" newCoach stats)"
    },
    {
      "name": "should root for teams that have more wins than losses",
      "task_id": 4,
      "status": "pass",
      "message": null,
      "output": null,
      "test_code": "Team \"\" (Coach \"\" True) (Stats 1 0) |> rootForTeam\n |> Expect.equal True"
    },
    {
      "name": "should not root for teams that lose more than they win",
      "task_id": 4,
      "status": "pass",
      "message": null,
      "output": null,
      "test_code": "Team \"\" (Coach \"\" True) (Stats 43 44) |> rootForTeam\n |> Expect.equal False"
    },
    {
      "name": "should not root for teams that have equal losses and wins",
      "task_id": 4,
      "status": "pass",
      "message": null,
      "output": null,
      "test_code": "Team \"\" (Coach \"\" True) (Stats 143 143) |> rootForTeam\n |> Expect.equal False"
    },
    {
      "name": "should root for extended teams that have more wins than losses",
      "task_id": 4,
      "status": "pass",
      "message": null,
      "output": null,
      "test_code": "{name = \"\", coach = Coach \"\" True, stats = Stats 1 0, someOtherField = \"\"} |> rootForTeam\n |> Expect.equal True"
    }
  ]
}

Copy link
Contributor

@ceddlyburge ceddlyburge left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't checked that all the "describe" numbers accurately reflect the tasks in the documentation, but otherwise everything looks good.

@jiegillet
Copy link
Contributor Author

Thank you!
I will double check the tasks number from the instructions tomorrow and merge if it's all good.

@mpizenberg
Copy link
Member

Thanks @ceddlyburge for having a look! 🙏

Yes I meant to check that all the describe numbers correspond to the right task IDs.

@jiegillet
Copy link
Contributor Author

I double checked everything, all good, I'm merging :)

@jiegillet jiegillet merged commit 42a312e into exercism:main Mar 28, 2022
@jiegillet jiegillet deleted the jie-taskid branch March 28, 2022 01:55
Copy link
Member

@ErikSchierboom ErikSchierboom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! This is also an example where the smoke tests would be incredibly useful! Just to check: are there existing exercises that have a "describe N" where they should not point to a task?

@jiegillet
Copy link
Contributor Author

Yes, now that these are merged, I'll create smoke tests with them.

And no, at the moment, there are no other exercises that have a describe "N", it's quite unlikely to happen in my opinion, but we could also have a CI check for that:

grep 'describe \"\d\"' exercises/**/Tests.elm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:module/concept-exercise Work on Concept Exercises x:size/large Large amount of work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants