exercism / elm-test-runner

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

Update `elm-explorations/test` #47

Closed jiegillet closed 1 year ago

jiegillet commented 1 year ago

Closes #42

This PR updates what is necessary to update to v2 of elm-explorations/test:

I also added a CI check that is using the script in exercism/elm to run the tests on all exercises.

I expect that script to fail right now, because the reference elm.json in the main branch is using elm-explorations/test v1, which is incompatible with the new elm-test-rs. What I will do next (in a little while) is hardcode Dockerfile and the CI to run on the PR branch in https://github.com/exercism/elm/pull/551. That should pass the CI, and give us confidence that this can be merged. And then I will undo the hardcoding. Once the other PR is merged, I will re-trigger the CI here.

Hopefully that all works out.

jiegillet commented 1 year ago

Yay! After some CI fixes and and test flakiness, Things worked as expected in 0cf34f6. I therefore reverted to pointing to main. CI failure is expected.

I am now fairly confident things will work, so the plan is

  1. Merge https://github.com/exercism/elm/pull/551 (already approved, and timing is not critical because we checked that merging it will not break anything on the site)
  2. Re-run the CI over here, should pass (flakiness still possible)
  3. Merge this

It would be fantastic if those 3 steps could be done in close succession, even if the timing is not critical. After this has been reviewed properly, I'm thinking of asking @ErikSchierboom to do them. Erik would that be cool with you?

ErikSchierboom commented 1 year ago

@jiegillet Sure. Just to be clear: you can't re-run workflows yourselves?

jiegillet commented 1 year ago

@ErikSchierboom actually, I feel silly, I can do those things... The only thing we cannot do is approve the PR. But I would like to have a review from @mpizenberg or @ceddlyburge first.

mpizenberg commented 1 year ago

I can review next weekend I think, or maybe earlier if I find time. (Don't hesitate to ping me Friday so that I don't forget)

mpizenberg commented 1 year ago

At a glance, this looks correct, but I just read it through the code on my phone

ceddlyburge commented 1 year ago

This all looks good to me as well, although it is hard / impossible to verify just by looking at it. I think merging that first pull request and checking that the build passes is probably the way to get confidence that it will work.

jiegillet commented 1 year ago

@mpizenberg could you take a look this weekend?

jiegillet commented 1 year ago

@ErikSchierboom after merging the other PR, the CI passes here without an issue, and everything seems fine on the website too. Could you approve so I can merge?

jiegillet commented 1 year ago

it all seems to work, thank you all!