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

Allow for custom test class names #41

Merged
merged 6 commits into from
Jan 18, 2025

Conversation

artamonovkirill
Copy link
Contributor

Not all tests are executed (the @Ignore annotation is not removed) if the test class name is different from the slug name. For example, for linked-list the expected slug-based test class name is LinkgedListSpec, and the actual test class name is DoubleLinkedListSpec
This PR changes the logic to try fetching the class name from .meta/config.json first and fall back to the slug-derived test class name if it fails.

Similar to exercism/scala-test-runner#38

@artamonovkirill artamonovkirill requested a review from a team as a code owner January 11, 2025 14:11
Copy link

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.

@github-actions github-actions bot closed this Jan 11, 2025
@vaeng vaeng reopened this Jan 11, 2025
@vaeng
Copy link

vaeng commented Jan 11, 2025

I don't know enough groovy to do more than reopening this pr. If you have not done so already you might find some better informed maintainers on the forums.

@artamonovkirill
Copy link
Contributor Author

I don't know enough groovy to do more than reopening this pr. If you have not done so already you might find some better informed maintainers on the forums.

The only Groovy code is an equivalent of assert false (= always fail) test; the rest is bash script changes. Maybe @IsaacG can share some improvements? 🙏

@@ -0,0 +1,183 @@
#!/usr/bin/env sh
Copy link
Member

Choose a reason for hiding this comment

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

Is there any particular reason this uses sh and not bash?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤷 gradlew is a wrapper file generated by Gradle; I just copied and pasted the existing one from other directories.
But your question got me wondering if we need any gradle files in the tests/ subfolders - as the test runner uses Maven, not Gradle. Let me try removing the wrappers with build.gradle files - it might be just some remaining artifacts from earlier development.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it indeed is:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tested locally without gradle files - it works, as expected; push the commit to this PR

@artamonovkirill
Copy link
Contributor Author

artamonovkirill commented Jan 12, 2025

@IsaacG sorry I didn't mention anywhere that gradlew is an autogenerated file that I copy-pasted from the other test, and you had to review it 😞
Here's the source of it: https://github.com/gradle/gradle/blob/master/gradlew (although the one in the test runner was probably a way older version)

@IsaacG
Copy link
Member

IsaacG commented Jan 18, 2025

@IsaacG sorry I didn't mention anywhere that gradlew is an autogenerated file that I copy-pasted from the other test, and you had to review it 😞

Not a big deal :)

@IsaacG
Copy link
Member

IsaacG commented Jan 18, 2025

Approved. Let me know if you lack merge permissions and need help.

@artamonovkirill
Copy link
Contributor Author

Approved. Let me know if you lack merge permissions and need help.

Thank you 🙌 I indeed do not have merge permissions 😞

@IsaacG IsaacG merged commit 744da25 into exercism:main Jan 18, 2025
1 check passed
@artamonovkirill artamonovkirill deleted the fix/custom-class-names branch January 18, 2025 12:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants