-
-
Couldn't load subscription status.
- Fork 7
Update test runner to Groovy 4 #40
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
Conversation
- Groovy 4 - Spock 2.3 - A script to verify reference solutions work
|
Hello. Thanks for opening a PR on Exercism 🙂 We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in. You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch. If you're interested in learning more about this auto-responder, please read this blog post. Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it. |
|
|
||
| # Assumes exercism/groovy repository is checked out next to the exercism/groovy-test-runner repository. | ||
| # If the repository is checked out elsewhere, provide the path as the first argument. | ||
| groovy_repo_path=$(realpath "${1:-${PWD}/../groovy}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is realpath standard? I'm used to readlink -e
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used the same command as the existing run-tests.sh: https://github.com/artamonovkirill/groovy-test-runner/blob/86676ee7822a6bb22b9a366887a8383bf4d10137/bin/run-tests.sh#L19
Reading this thread, readlink might have been more widely available in 2014, but I am not sure about now.
I'd opt for realpath for consistency.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, seems like realpath is used more often in the Exercism repos:
| results_json="${exercise_tmp_dir}/output/results.json" | ||
| status=$(jq -r ".status" "${results_json}") | ||
| if [[ "${status}" != "pass" ]]; then | ||
| echo "💥 Test failed for ${exercise_slug}, check the output in ${results_json}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to run all the tests to get the full details vs abort at the first failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's OK to fail on the first one - at least for now, as it's easier implementation-wise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
failures=()
for ...;
if failed; then failures+=("${slug}"); fi
done
if [[ "${failures}" ]]; then ...; exit 1; fi
exit 0There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing, updated!
And remove redundant use of basename
Use numbers comparison Co-authored-by: Isaac Good <[email protected]>
| exit 1 | ||
| fi | ||
|
|
||
| echo "✅ All tests passed" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are the reference solutions passing with Groovy 4?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, at least on my PC (c)
docker rmi -f $(docker images -a -q)docker system prune./bin/run-reference-solution-tests-in-docker.shoutput gist pulls Spock 2.3 and Groovy 4.0.24
Should we add the step to check reference solutions to the CI? However, we will need first to check out the exercism/groovy repository.
Or would someone be up to running the same ☝️ on their machine?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think any of that is necessary. After this is merged, you'll probably want to update the GitHub CI workflows in the exercism/groovy repo, so we'll catch it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, the plan was to update the exercise/groovy to the same Groovy and Spock version next.
|
FYI, the test runner still works on the website, as expected: success on Hello World
failed tests on Hello World |
|
Here's the one for exercism/groovy: exercism/groovy#468 |
LOL, so timid. |
Groovy 4
Changes
Comments
groovy-test-runnercaches its dependencies via Maven and can be upgraded independently from the Gradle dependencies in the groovy repository.groovy-allto justgroovy, but that should be fine since the exercises should depend on core language features, not on extra libraries (e.g.,groovy-jsonorgroovy-xml). At least the reference solutions do not - seerun-reference-solution-tests-in-docker.shscript and the proof of work for more details.run-reference-solution-tests-in-docker.shrelies on an external repository (groovy) checked out, I only ran it manually but omitted adding it to CIProof of work
./bin/run-reference-solution-tests-in-docker.sh