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

Use Guava testlib for collection tests #2746

Merged

Conversation

Marcono1234
Copy link
Collaborator

Purpose

Use the Guava collection testlib for Gson's own collection classes

Description

Use the Guava collection testlib for Gson's own collection classes NonNullElementWrapperList and LinkedTreeMap, covered through the tests for JsonArray#asList and JsonObject#asMap, as well as a test for LinkedTreeMap.

I don't have that much experience with the Guava testlib yet, so any feedback or recommendations are appreciated!

Checklist

  • New code follows the Google Java Style Guide
    This is automatically checked by mvn verify, but can also be checked on its own using mvn spotless:check.
    Style violations can be fixed using mvn spotless:apply; this can be done in a separate commit to verify that it did not cause undesired changes.
  • If necessary, new public API validates arguments, for example rejects null
  • New public API has Javadoc
    • Javadoc uses @since $next-version$
      ($next-version$ is a special placeholder which is automatically replaced during release)
  • If necessary, new unit tests have been added
    • Assertions in unit tests use Truth, see existing tests
    • No JUnit 3 features are used (such as extending class TestCase)
    • If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
  • mvn clean verify javadoc:jar passes without errors

Copy link
Member

@cpovirk cpovirk left a comment

Choose a reason for hiding this comment

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

Thanks also for the Guava bug report, which I might not look at in detail for a while but which sounds concerning.

CollectionFeature.ALLOWS_NULL_QUERIES,
CollectionFeature.RESTRICTS_ELEMENTS, // List only allows JsonElement
CollectionFeature.SUPPORTS_ADD,
ListFeature.REMOVE_OPERATIONS,
Copy link
Member

Choose a reason for hiding this comment

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

We typically use the GENERAL_PURPOSE features when applicable. There's such a constant for List and also for Map. I haven't confirmed whether it applies here, and it's fine to use an explicit list instead even if it would apply, but I wanted to at least point it out.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the hint! I wasn't completely sure whether they were suitable because CollectionFeature.GENERAL_PURPOSE says:

Features supported by general-purpose collections - everything but RESTRICTS_ELEMENTS.

See java.util.Collection the definition of general-purpose collections.

And the List and Map here are not 'general-purpose' in the sense of the java.util.Collection definition.

@eamonnmcmanus eamonnmcmanus merged commit 3741287 into google:main Oct 9, 2024
11 checks passed
@Marcono1234 Marcono1234 deleted the marcono1234/guava-collection-testlib branch October 9, 2024 20:30
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.

3 participants