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 ut for scala command arguments #588

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

gzm55
Copy link
Contributor

@gzm55 gzm55 commented Apr 4, 2022

supplement ut pr #586 as @slandelle suggest here

Copy link
Collaborator

@slandelle slandelle left a comment

Choose a reason for hiding this comment

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

I'm not fond of widening the visibility to "protected" for the sake of testing, because I've seen some people jumping into this loop hole and extend the components (yes, even with maven mojos).

IMHO, we should have the mojo do the maven plumbing, but we could resort to package protected static methods to implement the actual logic (that's what I did for "targetOption").

@gzm55
Copy link
Contributor Author

gzm55 commented Apr 4, 2022

the getScalaCommand is not easy to test or extract to a static method, it depends on the deep logic of the mojo. what's more, the tests on core static methods cannot cover the whole execution path, we still need some way to explode the field or method to the test, such as mock, override, widing visibility or even refelction.

@slandelle
Copy link
Collaborator

getScalaCommand doesn't do any side effect. It's just that it relies on instance fields.

We could introduce a getScalaCommand0 package protected static method that takes all the necessary parameters and have the mojo instance method call it and pass the instance fields.

@gzm55 gzm55 requested a review from slandelle April 4, 2022 16:51
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.

2 participants