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

Replace bash JavaTest.sh with mvn test #7500

Merged
merged 8 commits into from Sep 12, 2022

Conversation

nick-someone
Copy link
Member

This moves the pom.xml into java/, and sets it up as an independently built tool.

I symlinked the operative directories that were needed from tests/, and copied the serialized .mon file so it would be loaded as a test resource.

@dbaileychess
Copy link
Collaborator

@paulovap FYI

@paulovap
Copy link
Collaborator

paulovap commented Sep 1, 2022

I see it as a positive change and welcome, but I must point out there was a original intent from @aardappel to keep dependencies to a minimum. In this case two dependencies are added (JUnit and Truth for test only).

In any case LGTM after my comments are resolved.

@dbaileychess
Copy link
Collaborator

I'm OK for adding dependencies to tests, especially if they are automatically resolved. I assume Maven is pretty standard in java development that it isn't a burden for people?

Adding dependencies for the runtime is a bit more case-to-case.

@aardappel
Copy link
Collaborator

I guess dependencies are ok as long as they're only needed by the person making changes to that language's implementation, and like you say, they are pretty standard.

The only complication is that it becomes harder to run "all tests" the more stuff you need to have installed for it.

@dbaileychess
Copy link
Collaborator

@nick-someone there are few pending comments to address

@dbaileychess dbaileychess merged commit 89dfb43 into google:master Sep 12, 2022
@dbaileychess
Copy link
Collaborator

@nick-someone Thanks a bunch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI Continuous Integration java
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants