Clean up Gson source code

[eamonnmcmanus]

We've been reluctant to make large-scale changes because they are likely to cause pain for people with unmerged PRs. But at this point I think there are relatively few open PRs that we are likely to merge, thanks largely to the heroic work of @Marcono1234. So this would be as good a time as any to consider at least:

• Port tests from JUnit 3 to JUnit 4. (Google's internal systems don't support JUnit 5 so we can't use that.)
• Use Truth for clearer assertions in tests.
• Apply various automated fixes suggested by Error Prone and the like.
• Reformat all source files with google-java-format. Possibly also install a check that ensures that future PRs are also correctly formatted.

[MaicolAntali]

For me, sound very good. If you need a help with the port of the tests or with the code reformat I'm available.

[Marcono1234]

Reformat all source files with google-java-format

We should probably create a .git-blame-ignore-revs file then to avoid that commit from appearing in git blame (and on GitHub's "Blame" feature). That would require that the formatting changes would have to be in its own pull request and not be combined with any other clean up (or the commits of the pull request should not be squashed).

Port tests from JUnit 3 to JUnit 4

Maybe some of the changes from #2048 could be useful for that since JUnit 4 and JUnit 5 API is pretty similar. Though on the other hand it might be easier to port the tests from scratch since that pull request likely has multiple conflicts by now.

Apply various automated fixes suggested by Error Prone and the like.

Maybe then also configure Error Prone to run as part of the build?

[eamonnmcmanus]

We should probably create a .git-blame-ignore-revs file then to avoid that commit from appearing in git blame

Excellent idea! I think we should probably do this reformatting last.

Maybe then also configure Error Prone to run as part of the build?

That also seems like a great idea.

One other thing I forgot to mention: change the test assertions to use Truth.

[eamonnmcmanus]

Some ideas on how to migrate JUnit 3 tests to JUnit 4 are here.

[MaicolAntali]

Before formatting all the code i suggest to do another code clean up because there are so many things that can be corrected, like:

• Some methods / tests have redundant throws clause
• There are redundant / unused local variable
• Unnecessary boxing / unboxing, call to toString(), modifier, semicolon
• There are some Java API marked for removal. (AccessController is deprecated and marked for removal since Java 17)
• Some anonymous type can be replaced with lambda
• Identical catch in try statement can be collapse in a single catch block
• Some explicit type can be replace with <>
• ...

[eamonnmcmanus]

Yes. ErrorProne would catch some of those, and other automated analyzers might catch others.

We still build for Java 7, so lambdas are not available unfortunately. The main reason for keeping Java 7 is for Android, and I believe current tooling uses "desugaring" to allow lambdas and certain Java 8+ APIs. So we might be able to lift that restriction at some point. One thing at a time, though.

[MaicolAntali]

Is there anything that automates the switch from Junit asserts from the Truth asserts?

[Marcono1234]

Also, is the intention here to switch most assertions to Truth? Personally I am not completely conviced by this idea to switch to Truth yet (disclaimer: I haven't used Truth yet).

For more complex assertions such as assertions on strings or collections I guess it would indeed be useful but I am not sure if using it for something like assertThat(a).isEqualTo(b) or assertThat(c).isTrue() is really worth it compared to built-in JUnit assertions. Especially since this might also slightly increase the work for new first time contributors who are not familiar with Truth.

Would it make more sense to integrate Truth more and more over time (and also only for complex assertions?) but not try to convert everything at once. If I recall correctly some of Gson's tests are somewhat verbose and rewriting them all at once and then also making sure that they still cover everything correctly might be a bit too troublesome.

[MaicolAntali]

Gson's tests are somewhat verbose and rewriting them all at once and then also making sure that they still cover everything correctly might be a bit too troublesome

Yes, i agree with you.

For that reason I asked if there is some tool/regex/... to automate the switch. Otherwise, yes, it is not worth switch everything manually to Thust (it needs too much effort).

[eamonnmcmanus]

I agree that assertThat(condition).isTrue() is no better than assertTrue(condition), and maybe a bit worse. For the others, though, I think assertThat(foo).isEqualTo(bar) is a lot better than assertEquals(bar, foo), because it's much clearer which is the expected value and which is the actual one. I would not be surprised to find that some existing Gson assertions get these parameters the wrong way around. The reason it matters is that if the assertion fails you will get a message like expected <bar> but was <foo>, and that will be confusing if it is foo that is the expected value.

I've converted tests to Truth in the past using regexes, and I don't recommend it. I think it's possible to use Error Prone to patch in this change, because there are standard Error Prone refactorings to use Truth. But I don't know exactly how to do that.

Anyway this isn't particularly urgent.

[MaicolAntali]

If I'm not wrong, the last thing remains in this issue to do is the last point:

Reformat all source files with google-java-format. Possibly also install a check that ensures that future PRs are also correctly formatted.

After some searches, I found a GitHub action (googlejavaformat-action) that checks if the formatting of the code follow the Google Java Style Guide (The action could automatically format the java files, but I don’t think you want this).

This could be the format.yaml action file:

name: Format

on: pull_request

jobs:
  formatting:
    runs-on: ubuntu-latest
    steps:
      - uses: actions/checkout@v3
     - uses: axel-op/googlejavaformat-action@v3
        with:
          args: "--replace"
          skip-commit: true
      - name: Print diffs
        run: git --no-pager diff --exit-code

I never used this action, so I don't know if works fine. What do you think? Do you have some other "trusted" action to accomplish this job?

[Marcono1234]

Or possibly one of the Maven plugins listed on https://github.com/google/google-java-format#third-party-integrations, such as Spotless? And then invoke that either as separate step or job from the build.yml GitHub workflow (or have a separate workflow)?

These Maven plugins normally have separate goals for checking the formatting, and reformatting the code. Then users could also reformat the code themselves locally from the command line.

[Marcono1234]

Probably we can close this issue now since all the points in the description have been addressed? Thanks also to @MaicolAntali's great work!

The only open point might be using Truth also for the other Maven modules (such as extras), it seems to be currently only used for the gson module. For any remaining smaller clean-up we can probably just directly open pull requests for it.

[eamonnmcmanus]

Yes, I think this issue has served its purpose and can be closed.

[Marcono1234]

I have created #2687 now to migrate the remaining modules to Truth as well.
Hopefully no one else was working concurrently on that.