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

Introduce AssertThatMapContainsOnlyKeys Refaster rule #576

Merged
merged 2 commits into from
Apr 18, 2023

Conversation

mohamedsamehsalah
Copy link
Contributor

@mohamedsamehsalah mohamedsamehsalah commented Apr 10, 2023

Original suggestion here.

Suggested commit message:

Introduce `AssertThatMapContainsOnlyKeys` Refaster rule (#576)

@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@Stephan202 Stephan202 added this to the 0.10.0 milestone Apr 10, 2023
Copy link
Member

@Stephan202 Stephan202 left a comment

Choose a reason for hiding this comment

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

Added a small commit and tweaked the suggested commit message. Nice:)

Comment on lines 225 to 236
@BeforeTemplate
AbstractAssert<?, ?> before(Map<K, V> map, Set<K> keys) {
return assertThat(map.keySet()).hasSameElementsAs(keys);
}

@AfterTemplate
@UseImportPolicy(STATIC_IMPORT_ALWAYS)
AbstractAssert<?, ?> after(Map<K, V> map, Set<K> keys) {
return assertThat(map).containsOnlyKeys(keys);
}
Copy link
Member

Choose a reason for hiding this comment

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

Let's change the return types to make clear that what we're doing here (like with many of the other rules) potentially breaks some code.

@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@@ -131,7 +131,7 @@ MapAssert<Integer, Integer> testAbstractMapAssertHasSameSizeAs() {
}

AbstractAssert<?, ?> testAssertThatMapContainsOnlyKeys() {
return assertThat(ImmutableMap.of(1, 2).keySet()).hasSameElementsAs(ImmutableSet.of(1));
return assertThat(ImmutableMap.of(1, 2).keySet()).hasSameElementsAs(ImmutableSet.of(3));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks 🚀
I have seen this pattern everywhere but I cant fully understand the reasning behind this.
Why arent we using correct expected values ? 🤔

Copy link
Member

Choose a reason for hiding this comment

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

Good question. I wouldn't say we're fully consistent in this respect. But in new code I try to use distinct values, so that we properly validate that Refaster correctly "shuffles things around". (And since the code isn't executed anyway... 🙃.)

@rickie rickie force-pushed the mohamedsamehsalah/contains-only-keys branch from 399f7d4 to fca2848 Compare April 15, 2023 15:00
@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

Copy link
Member

@rickie rickie left a comment

Choose a reason for hiding this comment

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

Changes LGTM! Nice, another contribution @mohamedsamehsalah 🚀!

@werli do you want to take a look as well 😄?

@mohamedsamehsalah mohamedsamehsalah force-pushed the mohamedsamehsalah/contains-only-keys branch from fca2848 to 23fe7c6 Compare April 16, 2023 20:12
@github-actions
Copy link

Looks good. No mutations were possible for these changes.
Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

@rickie rickie changed the title Prefer usages of containsOnlyKeys when asserting keySets of a map Introduce AssertThatMapContainsOnlyKeys Refaster rule Apr 18, 2023
@rickie rickie merged commit ebd64c1 into master Apr 18, 2023
@rickie rickie deleted the mohamedsamehsalah/contains-only-keys branch April 18, 2023 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants