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 tests for equals() and forEach() on keySet() and entrySet(). #1678

Merged
merged 1 commit into from
Oct 16, 2024

Conversation

motlin
Copy link
Contributor

@motlin motlin commented Aug 21, 2024

No description provided.

@mohrezaei
Copy link
Member

With the recent changes to make Entry objects mutable, should we add tests that make sure immutable maps and synchronized maps act correctly if someone tries to mutate with forEach?

@motlin
Copy link
Contributor Author

motlin commented Aug 25, 2024

With the recent changes to make Entry objects mutable, should we add tests that make sure immutable maps and synchronized maps act correctly if someone tries to mutate with forEach?

For sure! For context, I started working on #500 about 3 months ago, and I have lots of changes staged on a few branches, including lots of additional test coverage.

When I put stuff like this up for review, the reviews are too big to get anyone to read through. I'm finding them difficult to split up and land separately as well. Lots of the assertions that I pull up to superclasses expose real differences between the maps. None of them are earth shattering, but they slow me down.

  • Whether Entries support mutation, which in turn affects whether default methods like Map.replace() throw
  • Which exception is thrown when null is passed in
  • Whether a method that may perform mutation like getIfAbsentPut always throws or only throws on mutation
  • Other stuff that I'm forgetting now.

I'm confident that immutable maps will never mutate through Entries, but it's possible they throw different exceptions from different maps.

While I haven't pulled up tests of setValue() into the interface-based tests yet, there is already some coverage in the old-style tests.

assertThrows(UnsupportedOperationException.class, () -> Iterate.getFirst(map.entrySet()).setValue("Three"));

@motlin motlin force-pushed the map-test-coverage branch 2 times, most recently from 976235f to 3234c1a Compare August 31, 2024 16:52
@motlin motlin force-pushed the map-test-coverage branch 2 times, most recently from 3cefbab to 47ba99a Compare September 10, 2024 22:06
@motlin motlin force-pushed the map-test-coverage branch 4 times, most recently from 610a31f to 3a9cf81 Compare September 29, 2024 17:52
@motlin
Copy link
Contributor Author

motlin commented Oct 5, 2024

This one has been open for a while and just adds test coverage. Any objections to landing it?

@motlin motlin force-pushed the map-test-coverage branch 2 times, most recently from 03f3227 to b2c6180 Compare October 13, 2024 16:20
@motlin motlin merged commit 913adc0 into eclipse:master Oct 16, 2024
18 checks passed
@motlin motlin deleted the map-test-coverage branch October 16, 2024 00:20
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