Use google-java-format in spotless #1934

TimvdLippe · 2020-05-18T11:24:58Z

Google-java-format is an open source formatter 1. It automatically formats
source code based on the Google Java Style.

The Mockito source code to a very large extent already adheres to this style
guide. While this PR in itself is large, many of the changes are related to
string formatting and nested method calls. Most notably, google-java-format
is an improvement over the current formatting strategy in that:

It handles comment formatting (e.g. spacing between comments)
It handles nested method calls. You can see the difference in
our usage of the ByteBuddy API, which is now more consistent
It enforces the max-line length.

It essentially automates all of the styling rules we list in
https://github.com/mockito/mockito/blob/release/3.x/.github/CONTRIBUTING.md#alignment
As such, for new contributors it should be a lot easier (and less scary)
to contribute to Mockito, as they no longer have to be concerned about
formatting. Hopefully, this once again lowers the bar for external contributors
who want to help the project, but would otherwise feel overwhelmed by
the rules we have to adhere to. (If we wouldn't have these rules, it would
be a lot harder for us to maintain a consistent and maintainable codebase).

The only interesting changes in this PR are those in build.gradle. All
other changes were auto-generated by running ./gradlew spotlessApply.

Note that I disabled the formatting of Javadoc, as I think we should keep formatting
that ourselves. We normally put a lot of time and effort in our Javadoc and changing
that all at once seems like the wrong decision at this point in time.

Google-java-format is an open source formatter [1]. It automatically formats source code based on the Google Java Style. The Mockito source code to a very large extent already adheres to this style guide. While this PR in itself is large, many of the changes are related to string formatting and nested method calls. Most notably, google-java-format is an improvement over the current formatting strategy in that: 1. It handles comment formatting (e.g. spacing between comments) 2. It handles nested method calls. You can see the difference in our usage of the ByteBuddy API, which is now more consistent 3. It enforces the max-line length. It essentially automates all of the styling rules we list in https://github.com/mockito/mockito/blob/release/3.x/.github/CONTRIBUTING.md#alignment As such, for new contributors it should be a lot easier (and less scary) to contribute to Mockito, as they no longer have to be concerned about formatting. Hopefully, this once again lowers the bar for external contributors who want to help the project, but would otherwise feel overwhelmed by the rules we have to adhere to. (If we wouldn't have these rules, it would be a lot harder for us to maintain a consistent and maintainable codebase). The only interesting changes in this PR are those in `build.gradle`. All other changes were auto-generated by running `./gradlew spotlessApply`. Note that I disabled the formatting of Javadoc, as I think we should keep formatting that ourselves. We normally put a lot of time and effort in our Javadoc and changing that all at once seems like the wrong decision at this point in time. [1]: https://github.com/google/google-java-format

TimvdLippe · 2020-05-18T13:53:36Z

Ah I see GJF requires Java 11 to run, but we run on older versions of Java. We could configure Travis to only run the check on Java 11, since our formatting is not Java-version specific.

TimvdLippe · 2020-06-14T18:44:49Z

@mockitoguy @raphw @bric3 Do you have any objections/thoughts on this PR? If you don't have any, I can polish the PR and make sure Travis happy and all.

mockitoguy

Looks good. Make Travis happy and ship it!

I absolutely agree with the motivation behind this PR.

THANK YOU!

bric3

While I think these chages are great in general, there's part of the code where the auto format is huritng readability I think. Is there a way to tweak a bit for these cases.

I've highlighted a few issues, but probably missed some.

bric3 · 2020-06-15T10:09:02Z

src/main/java/org/mockito/internal/configuration/injection/SpyOnInjectedFieldsHandler.java

+                                    withSettings()
+                                            .spiedInstance(instance)
+                                            .defaultAnswer(Mockito.CALLS_REAL_METHODS)
+                                            .name(field.getName()));


The above was easier to read imho.

bric3 · 2020-06-15T10:09:45Z

src/main/java/org/mockito/internal/configuration/injection/PropertyAndSetterInjection.java

+            Object injected =
+                    mockCandidateFilter
+                            .filterCandidate(
+                                    mocks, candidateField, orderedCandidateInjecteeFields, injectee)


Would it be possible to vertically allign parameters, instead. This change is weird to read.

bric3 · 2020-06-15T11:34:21Z

...in/java/org/mockito/internal/configuration/injection/filter/TerminalMockCandidateFilter.java

+            final Collection<Object> mocks,
+            final Field candidateFieldToBeInjected,
+            final List<Field> allRemainingCandidateFields,
+            final Object injectee) {


I also think the above was a tad better before.

bric3 · 2020-06-15T11:35:03Z

src/main/java/org/mockito/internal/configuration/plugins/PluginLoader.java

-        this(new DefaultMockitoPlugins(), new PluginInitializer(pluginSwitch, null, new DefaultMockitoPlugins()));
+        this(
+                new DefaultMockitoPlugins(),
+                new PluginInitializer(pluginSwitch, null, new DefaultMockitoPlugins()));


Is it possible to fix this annoying new line there ?

TimvdLippe · 2020-06-15T12:39:23Z

Thanks @mockitoguy and @bric3 for the review. I will relay the feedback about the formatting output to the google-java-format team and get them take a look. Since on the overall this change is a plus, I think we can merge without the fixes, but I will make sure they will be followed up on. I agree with your comments, so we can probably make a good case for fixing 😄

I will address the Travis changes in a separate PR, as that requires some cleanup there as well. Then I will rebase this PR and regenerate the formatting changes.

Thanks for the quick response after the ping, greatly appreciated!

This means the tests can run in parallel of the formatting changes. Therefore, formatting changes would not block obtaining your test results, which should hopefully reduce the amount of Travis builds necessary to work on a community PR. This is also in preparation of #1934 which requires spotless to run on JDK11 minimum.

bric3 · 2020-06-17T09:11:33Z

👍

This means the tests can run in parallel of the formatting changes. Therefore, formatting changes would not block obtaining your test results, which should hopefully reduce the amount of Travis builds necessary to work on a community PR. This is also in preparation of #1934 which requires spotless to run on JDK11 minimum.

codecov-commenter · 2020-06-18T10:45:33Z

Codecov Report

Merging #1934 into release/3.x will decrease coverage by 0.44%.
The diff coverage is 81.21%.

@@                Coverage Diff                @@
##             release/3.x    #1934      +/-   ##
=================================================
- Coverage          86.21%   85.76%   -0.45%     
  Complexity          2542     2542              
=================================================
  Files                318      318              
  Lines               6738     7209     +471     
  Branches             838      861      +23     
=================================================
+ Hits                5809     6183     +374     
- Misses               715      810      +95     
- Partials             214      216       +2

Impacted Files	Coverage Δ	Complexity Δ
src/main/java/org/mockito/ArgumentCaptor.java	`100.00% <ø> (ø)`	`5.00 <0.00> (ø)`
src/main/java/org/mockito/Matchers.java	`0.00% <0.00%> (ø)`	`0.00 <0.00> (ø)`
...ito/configuration/DefaultMockitoConfiguration.java	`100.00% <ø> (ø)`	`5.00 <0.00> (ø)`
.../org/mockito/exceptions/base/MockitoException.java	`100.00% <ø> (ø)`	`4.00 <0.00> (ø)`
...ions/verification/junit/ArgumentsAreDifferent.java	`66.66% <ø> (ø)`	`1.00 <0.00> (ø)`
...verification/opentest4j/ArgumentsAreDifferent.java	`77.77% <ø> (ø)`	`2.00 <0.00> (ø)`
...ockito/internal/configuration/ClassPathLoader.java	`54.54% <0.00%> (-5.46%)`	`2.00 <0.00> (ø)`
...internal/configuration/DefaultInjectionEngine.java	`100.00% <ø> (ø)`	`2.00 <0.00> (ø)`
...l/configuration/injection/scanner/MockScanner.java	`95.23% <0.00%> (-0.22%)`	`13.00 <2.00> (ø)`
...a/org/mockito/internal/creation/SuspendMethod.java	`71.42% <ø> (ø)`	`5.00 <0.00> (ø)`
... and 209 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ddba092...f0cee3a. Read the comment docs.

TimvdLippe · 2020-06-18T10:47:32Z

Most of the coverage changes are related to having more lines in exception messages that are untested. Functionally, we haven't lost coverage. https://codecov.io/gh/mockito/mockito/pull/1934/changes also doesn't list any file, which leads me to that conclusion.

Therefore, I am landing this now. We can improve our code coverage in separate CLs (we are still at 85%, which is quite good)

This means the tests can run in parallel of the formatting changes. Therefore, formatting changes would not block obtaining your test results, which should hopefully reduce the amount of Travis builds necessary to work on a community PR. This is also in preparation of mockito#1934 which requires spotless to run on JDK11 minimum.

Google-java-format is an open source formatter [1]. It automatically formats source code based on the Google Java Style. The Mockito source code to a very large extent already adheres to this style guide. While this PR in itself is large, many of the changes are related to string formatting and nested method calls. Most notably, google-java-format is an improvement over the current formatting strategy in that: 1. It handles comment formatting (e.g. spacing between comments) 2. It handles nested method calls. You can see the difference in our usage of the ByteBuddy API, which is now more consistent 3. It enforces the max-line length. It essentially automates all of the styling rules we list in https://github.com/mockito/mockito/blob/release/3.x/.github/CONTRIBUTING.md#alignment As such, for new contributors it should be a lot easier (and less scary) to contribute to Mockito, as they no longer have to be concerned about formatting. Hopefully, this once again lowers the bar for external contributors who want to help the project, but would otherwise feel overwhelmed by the rules we have to adhere to. (If we wouldn't have these rules, it would be a lot harder for us to maintain a consistent and maintainable codebase). The only interesting changes in this PR are those in `build.gradle`. All other changes were auto-generated by running `./gradlew spotlessApply`. Note that I disabled the formatting of Javadoc, as I think we should keep formatting that ourselves. We normally put a lot of time and effort in our Javadoc and changing that all at once seems like the wrong decision at this point in time. [1]: https://github.com/google/google-java-format

TimvdLippe requested review from mockitoguy, bric3 and raphw May 18, 2020 11:24

mockitoguy approved these changes Jun 15, 2020

View reviewed changes

bric3 approved these changes Jun 15, 2020

View reviewed changes

TimvdLippe mentioned this pull request Jun 15, 2020

Move spotless check to separate build task #1946

Merged

TimvdLippe added 4 commits June 18, 2020 10:29

Merge branch 'release/3.x' into google-java-format

90747ce

Rerun formatter

ae0046a

Fix build

8d5f28d

Fix test that puts 2 statements on the same line

f0cee3a

TimvdLippe merged commit a0dd0b9 into release/3.x Jun 18, 2020

TimvdLippe deleted the google-java-format branch June 18, 2020 10:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use google-java-format in spotless #1934

Use google-java-format in spotless #1934

TimvdLippe commented May 18, 2020

TimvdLippe commented May 18, 2020

TimvdLippe commented Jun 14, 2020

mockitoguy left a comment

bric3 left a comment

bric3 Jun 15, 2020

bric3 Jun 15, 2020

bric3 Jun 15, 2020

bric3 Jun 15, 2020

TimvdLippe commented Jun 15, 2020

bric3 commented Jun 17, 2020

codecov-commenter commented Jun 18, 2020 •

edited

TimvdLippe commented Jun 18, 2020

Use google-java-format in spotless #1934

Use google-java-format in spotless #1934

Conversation

TimvdLippe commented May 18, 2020

TimvdLippe commented May 18, 2020

TimvdLippe commented Jun 14, 2020

mockitoguy left a comment

Choose a reason for hiding this comment

bric3 left a comment

Choose a reason for hiding this comment

bric3 Jun 15, 2020

Choose a reason for hiding this comment

bric3 Jun 15, 2020

Choose a reason for hiding this comment

bric3 Jun 15, 2020

Choose a reason for hiding this comment

bric3 Jun 15, 2020

Choose a reason for hiding this comment

TimvdLippe commented Jun 15, 2020

bric3 commented Jun 17, 2020

codecov-commenter commented Jun 18, 2020 • edited

Codecov Report

TimvdLippe commented Jun 18, 2020

codecov-commenter commented Jun 18, 2020 •

edited