Sort arguments of RefasterRuleCollectionTestCase#elidedTypesAndStaticImports by mohamedsamehsalah · Pull Request #850 · PicnicSupermarket/error-prone-support

mohamedsamehsalah · 2023-10-21T19:19:32Z

This PR is split into two commits:

Code implementation.
Manually applied suggestions

Suggested commit message:

Sort arguments of RefasterRuleCollectionTestCase#elidedTypesAndStaticImports (#850)

github-actions · 2023-10-21T19:21:49Z

Surviving mutants in this change: 5
Killed mutants in this change: 16

class	surviving	killed
🧟`tech.picnic.errorprone.refaster.test.RefasterRuleCollection$1`	3	9
🧟`tech.picnic.errorprone.refaster.test.RefasterRuleCollection`	2	5
🎉`tech.picnic.errorprone.refaster.test.RefasterRuleCollection$UnexpectedMatchReporter`	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarqubecloud · 2023-10-21T19:22:21Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

94.5% Coverage
0.0% Duplication

mohamedsamehsalah · 2023-10-21T19:21:37Z

...-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java

+                        methodInvocationTree.getArguments();
+
+                    ImmutableList<? extends ExpressionTree> desiredArgumentOrdering =
+                        actualArgumentsOrdering.stream()


Should we group argument types togther before sorting ?
i.e: Group & sort method invocation lexicographically > group & sort identifiers lexicographically, etc...

One tip for the ordering, in lexicographical sorting, the capital letters A-Z come before the a-z 😄.

Characters are sorted in lexicographic order. Sorting is case sensitive, that is, uppercase letters appear before lowercase letter in sorted data.

Should we group argument types togther before sorting ?

Do you have an example of the case you mean? In any case, it sounds like something we should have a test case for 😉.

Do you have an example of the case you mean?

My thought was (As an example)

Group & sort Objects (Strings for eg).

Group & sort Class statemens (*.class).

Group & sort method arguments.

Group & sort primitives.

Essentially turning

ImmutableSet.of(List.class, "A", assertDoesNotThrow(() -> null), Iterables.class, "a", 1)

to

ImmutableSet.of("A", "a", List.class, Iterables.class, assertDoesNotThrow(() -> null),1)

mohamedsamehsalah · 2023-10-21T19:22:54Z

...-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java

+                                              methodInvocationTree.getMethodSelect())),
+                                      ")"));
+
+                      fixBuilder.merge(SuggestedFix.replace(methodInvocationTree, suggestion));


Curious why running ./apply-error-prone-suggestions.sh did not fix the *(Input|Otput) tests here 🤔

So what we are doing in the RefasterRuleCollection, is reporting things "in" the output file, that is what we are also verifying in the refaster-test-support/src/test/resources/tech/picnic/errorprone/refaster/test/NonSortedElidedTypesAndStaticImportsTestRulesTestOutput.java file.

So e.g. in the MatchInWrongMethodRulesTest input file we try to see what is wrong and we report that in the MatchInWrongMethodRulesTest output file, while not doing anything with the content itself, we only add a warning as a comment.

So in the NonSortedElidedTypesAndStaticImportsTestRulesTestOutput file we should not change the output to be correct, we only need a nice comment explaining what is wrong. Instead of

/* ERROR: Unexpected token. */

(which also signals something is wrong and the code we add has likely a bug)
It should add something like:

/* * ERROR: The entries of the `elidedTypesAndStaticImports` method are not sorted lexicographically. The method should look like this: * public ImmutableSet<Object> elidedTypesAndStaticImports() { * return ImmutableSet.of( Assertions.class, ..... (a method with correct sorting) */

This thing is not a "regular" BugPattern. It tries to guide a developer when writing TestInput & TestOutput files by giving this warning :).

Thanks for explanation 🙇 🙇

rickie

I gave some context that should make some things a bit clearer, if there are any questions let us know 😄.

Nice that you opened another PR @mohamedsamehsalah 🚀 !

.../picnic/errorprone/refaster/test/NonSortedElidedTypesAndStaticImportsTestRulesTestInput.java

rickie · 2023-10-22T07:52:32Z

...-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java

+                        methodInvocationTree.getArguments();
+
+                    ImmutableList<? extends ExpressionTree> desiredArgumentOrdering =
+                        actualArgumentsOrdering.stream()


One tip for the ordering, in lexicographical sorting, the capital letters A-Z come before the a-z 😄.

Characters are sorted in lexicographic order. Sorting is case sensitive, that is, uppercase letters appear before lowercase letter in sorted data.

Should we group argument types togther before sorting ?

Do you have an example of the case you mean? In any case, it sounds like something we should have a test case for 😉.

...-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java

rickie · 2023-10-22T08:04:08Z

...-test-support/src/main/java/tech/picnic/errorprone/refaster/test/RefasterRuleCollection.java

+                                              methodInvocationTree.getMethodSelect())),
+                                      ")"));
+
+                      fixBuilder.merge(SuggestedFix.replace(methodInvocationTree, suggestion));


So what we are doing in the RefasterRuleCollection, is reporting things "in" the output file, that is what we are also verifying in the refaster-test-support/src/test/resources/tech/picnic/errorprone/refaster/test/NonSortedElidedTypesAndStaticImportsTestRulesTestOutput.java file.

So e.g. in the MatchInWrongMethodRulesTest input file we try to see what is wrong and we report that in the MatchInWrongMethodRulesTest output file, while not doing anything with the content itself, we only add a warning as a comment.

So in the NonSortedElidedTypesAndStaticImportsTestRulesTestOutput file we should not change the output to be correct, we only need a nice comment explaining what is wrong. Instead of

/* ERROR: Unexpected token. */

(which also signals something is wrong and the code we add has likely a bug)
It should add something like:

/* * ERROR: The entries of the `elidedTypesAndStaticImports` method are not sorted lexicographically. The method should look like this: * public ImmutableSet<Object> elidedTypesAndStaticImports() { * return ImmutableSet.of( Assertions.class, ..... (a method with correct sorting) */

This thing is not a "regular" BugPattern. It tries to guide a developer when writing TestInput & TestOutput files by giving this warning :).

github-actions · 2023-11-25T21:44:23Z

Surviving mutants in this change: 4
Killed mutants in this change: 18

class	surviving	killed
🧟`tech.picnic.errorprone.refaster.test.RefasterRuleCollection$1`	3	9
🧟`tech.picnic.errorprone.refaster.test.RefasterRuleCollection`	1	7
🎉`tech.picnic.errorprone.refaster.test.RefasterRuleCollection$UnexpectedMatchReporter`	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

sonarqubecloud · 2023-11-25T21:44:26Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
2 Code Smells

94.7% Coverage
0.0% Duplication

github-actions · 2024-05-08T14:09:17Z

Surviving mutants in this change: 4
Killed mutants in this change: 15

class	surviving	killed
🧟`tech.picnic.errorprone.refaster.test.RefasterRuleCollection$1`	3	9
🧟`tech.picnic.errorprone.refaster.test.RefasterRuleCollection`	1	4
🎉`tech.picnic.errorprone.refaster.test.RefasterRuleCollection$UnexpectedMatchReporter`	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

…ports

sonarqubecloud · 2024-05-24T11:31:39Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
94.7% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-05-24T11:31:56Z

Surviving mutants in this change: 4
Killed mutants in this change: 15

class	surviving	killed
🧟`tech.picnic.errorprone.refaster.test.RefasterRuleCollection$1`	3	9
🧟`tech.picnic.errorprone.refaster.test.RefasterRuleCollection`	1	4
🎉`tech.picnic.errorprone.refaster.test.RefasterRuleCollection$UnexpectedMatchReporter`	0	2

Mutation testing report by Pitest. Review any surviving mutants by inspecting the line comments under Files changed.

mohamedsamehsalah commented Oct 21, 2023

View reviewed changes

mohamedsamehsalah requested review from Stephan202 and rickie October 21, 2023 19:23

rickie reviewed Oct 22, 2023

View reviewed changes

mohamedsamehsalah force-pushed the mohamedsamehsalah/321-sort-entries-of-refasterrulecollectionTestCase branch from 24a8c9b to faaee6e Compare November 25, 2023 21:42

mohamedsamehsalah requested a review from rickie November 25, 2023 21:43

mohamedsamehsalah force-pushed the mohamedsamehsalah/321-sort-entries-of-refasterrulecollectionTestCase branch from faaee6e to 1593ff2 Compare May 8, 2024 14:06

mohamedsamehsalah and others added 4 commits May 24, 2024 13:29

Sort entries of RefasterRuleCollectionTestCase#elidedTypesAndStaticIm…

91a748e

…ports

Apply the suggestions....manually ?

eed508e

Address comments: Add test cases and cover more argument types

fc0eda1

Post rebase fixes

0e90279

rickie force-pushed the mohamedsamehsalah/321-sort-entries-of-refasterrulecollectionTestCase branch from 1593ff2 to 0e90279 Compare May 24, 2024 11:29

Conversation

mohamedsamehsalah commented Oct 21, 2023 • edited by rickie Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Oct 21, 2023

Uh oh!

sonarqubecloud bot commented Oct 21, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rickie left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Nov 25, 2023

Uh oh!

sonarqubecloud bot commented Nov 25, 2023

Uh oh!

github-actions bot commented May 8, 2024

Uh oh!

sonarqubecloud bot commented May 24, 2024

Quality Gate passed

Uh oh!

github-actions bot commented May 24, 2024

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

2 participants

mohamedsamehsalah commented Oct 21, 2023 •

edited by rickie

Loading

rickie left a comment •

edited

Loading