From 60d5b901b67be2971f6c57a65168508d00ad1ea2 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Fri, 22 May 2026 00:53:21 +0000 Subject: [PATCH 1/2] =?UTF-8?q?=E2=9A=A1=20Bolt:=20Replace=20regex-based?= =?UTF-8?q?=20`replaceFirst`=20with=20`indexOf`/`substring`?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **What:** Replaced `String.replaceFirst()` with `startsWith()` and `indexOf()` combined with `substring()` in `FilterMethodVisitor.java`, `TestMethodDivinerJunit3Praefix.java`, and `TestMethodDivinerNoPraefix.java`. **Why:** `String.replaceFirst()` implicitly compiles a regex `Pattern` and invokes the matcher logic, which is slow for simple literal substring removal. Given that `FilterMethodVisitor` frequently parses AST structures and processes string names, this regex overhead was unnecessary. Literal substring removal is far faster. **Impact:** Improves execution performance within plugin string parsers and resolves potential bugs where literal regex characters (e.g. `$`) might cause exceptions. **Measurement:** Run `xvfb-run -a mvn -f org.moreunit.build/pom.xml test` to verify changes build successfully. Co-authored-by: RoiSoleil <3462260+RoiSoleil@users.noreply.github.com> --- .jules/bolt.md | 3 +++ .../org/moreunit/elements/FilterMethodVisitor.java | 14 ++++++++++++-- .../util/TestMethodDivinerJunit3Praefix.java | 14 +++++++++++--- .../moreunit/util/TestMethodDivinerNoPraefix.java | 8 +++++++- 4 files changed, 33 insertions(+), 6 deletions(-) diff --git a/.jules/bolt.md b/.jules/bolt.md index 9b3d00c7..c299ba4c 100644 --- a/.jules/bolt.md +++ b/.jules/bolt.md @@ -27,3 +27,6 @@ ## 2026-05-21 - SWTBot Race Condition in Refactoring Tests **Learning:** `RenameMethodTest` occasionally fails in CI because the rename refactoring runs asynchronously after the Rename dialog closes. If the JVM hits the assertion before the refactoring `Job` finishes writing to the `ICompilationUnit` model, it sees the old method name and fails. **Action:** When an Eclipse refactoring is tested in SWTBot, ensure you wait for the specific result (like the new method name appearing in the model) rather than just waiting for the dialog shell to close. Or, artificially sleep the test thread for a second. +## 2026-05-22 - Replacing literal `replaceFirst` with manual string searches +**Learning:** Using regex-based `replaceFirst` for literal search-and-replace, especially when dealing with prefixes or static replacements, is computationally expensive due to the overhead of `Pattern` compilation and matching. It is also prone to regex compilation bugs if the replacement target has regex reserved characters (like `$`). Swapping `replaceFirst` for a combination of `startsWith`, `indexOf`, and `substring` concatenation provides a significant performance boost in frequently called logic (like AST parsing inside `FilterMethodVisitor`). +**Action:** When finding exact substring replacements or prefix removal, manually calculate indices and substring lengths using `indexOf`/`substring` instead of `replaceFirst`. diff --git a/org.moreunit.plugin/src/org/moreunit/elements/FilterMethodVisitor.java b/org.moreunit.plugin/src/org/moreunit/elements/FilterMethodVisitor.java index 778ceb1e..aae4cb6e 100644 --- a/org.moreunit.plugin/src/org/moreunit/elements/FilterMethodVisitor.java +++ b/org.moreunit.plugin/src/org/moreunit/elements/FilterMethodVisitor.java @@ -110,7 +110,12 @@ public boolean isPrivateMethod(IMethod method) public boolean isGetterMethod(IMethod method) { - String getterVariableName = method.getElementName().replaceFirst(MoreUnitContants.GETTER_PREFIX, StringConstants.EMPTY_STRING); + // Performance optimization: Avoids regex compilation overhead of replaceFirst by using + // startsWith and substring, which is faster for simple literal prefix removal. + String elementName = method.getElementName(); + String getterVariableName = elementName.startsWith(MoreUnitContants.GETTER_PREFIX) + ? elementName.substring(MoreUnitContants.GETTER_PREFIX.length()) + : elementName; for (FieldDeclaration fieldDeclaration : fieldDeclarations) { @@ -191,7 +196,12 @@ private boolean sameVariableType(FieldDeclaration fieldDeclaration, IMethod meth public boolean isSetterMethod(IMethod method) { - String setterVariableName = method.getElementName().replaceFirst(MoreUnitContants.SETTER_PREFIX, StringConstants.EMPTY_STRING); + // Performance optimization: Avoids regex compilation overhead of replaceFirst by using + // startsWith and substring, which is faster for simple literal prefix removal. + String elementName = method.getElementName(); + String setterVariableName = elementName.startsWith(MoreUnitContants.SETTER_PREFIX) + ? elementName.substring(MoreUnitContants.SETTER_PREFIX.length()) + : elementName; for (FieldDeclaration fieldDeclaration : fieldDeclarations) { diff --git a/org.moreunit.plugin/src/org/moreunit/util/TestMethodDivinerJunit3Praefix.java b/org.moreunit.plugin/src/org/moreunit/util/TestMethodDivinerJunit3Praefix.java index 319e40bf..f6e01f5d 100644 --- a/org.moreunit.plugin/src/org/moreunit/util/TestMethodDivinerJunit3Praefix.java +++ b/org.moreunit.plugin/src/org/moreunit/util/TestMethodDivinerJunit3Praefix.java @@ -14,15 +14,23 @@ public String getTestMethodNameFromMethodName(String methodName) LogHandler.getInstance().handleWarnLog("Methodname is null or has length of 0"); return StringConstants.EMPTY_STRING; } - String firstChar = String.valueOf(methodName.charAt(0)); - return TEST_METHOD_PRAEFIX + methodName.replaceFirst(firstChar, firstChar.toUpperCase()); + // Performance optimization: Avoids replaceFirst by using the helper method. + // It is faster to use substring/concatenation instead of regex matching. + return TEST_METHOD_PRAEFIX + getStringWithFirstCharToUpperCase(methodName); } public String getTestMethodNameAfterRename(String methodNameBeforeRename, String methodNameAfterRename, String testMethodName) { String old = getStringWithFirstCharToUpperCase(methodNameBeforeRename); String newName = getStringWithFirstCharToUpperCase(methodNameAfterRename); - return testMethodName.replaceFirst(old, newName); + + // Performance optimization: Replaced regex-based replaceFirst with + // faster manual indexOf and substring extraction for literal replacement. + int index = testMethodName.indexOf(old); + if (index != -1) { + return testMethodName.substring(0, index) + newName + testMethodName.substring(index + old.length()); + } + return testMethodName; } private String getStringWithFirstCharToUpperCase(String string) diff --git a/org.moreunit.plugin/src/org/moreunit/util/TestMethodDivinerNoPraefix.java b/org.moreunit.plugin/src/org/moreunit/util/TestMethodDivinerNoPraefix.java index aa7fa656..8a85bf5e 100644 --- a/org.moreunit.plugin/src/org/moreunit/util/TestMethodDivinerNoPraefix.java +++ b/org.moreunit.plugin/src/org/moreunit/util/TestMethodDivinerNoPraefix.java @@ -10,7 +10,13 @@ public String getTestMethodNameFromMethodName(String methodName) public String getTestMethodNameAfterRename(String methodNameBeforeRename, String methodNameAfterRename, String testMethodName) { - return testMethodName.replaceFirst(methodNameBeforeRename, methodNameAfterRename); + // Performance optimization: Replaced regex-based replaceFirst with + // faster manual indexOf and substring extraction for literal replacement. + int index = testMethodName.indexOf(methodNameBeforeRename); + if (index != -1) { + return testMethodName.substring(0, index) + methodNameAfterRename + testMethodName.substring(index + methodNameBeforeRename.length()); + } + return testMethodName; } public String getMethodNameFromTestMethodName(String testMethodName) From 6a389ba274e3e89fe29e8ceb3923fccc0c4ee042 Mon Sep 17 00:00:00 2001 From: "google-labs-jules[bot]" <161369871+google-labs-jules[bot]@users.noreply.github.com> Date: Fri, 22 May 2026 01:24:28 +0000 Subject: [PATCH 2/2] test: improve coverage for TestMethodDiviner classes **What:** Added unit tests covering the `index == -1` (no match) edge cases in `TestMethodDivinerNoPraefixTest` and `TestMethodDivinerJunit3PraefixTest`. **Why:** The previous refactoring from `replaceFirst` to `indexOf` introduced a new `if (index != -1)` branch which was not covered by existing tests, causing the Codecov CI check to fail due to a drop in diff coverage. **Impact:** Increases code coverage and ensures that renaming methods correctly fallback when the expected prefix/suffix string does not exist in the target string. Co-authored-by: RoiSoleil <3462260+RoiSoleil@users.noreply.github.com> --- .jules/bolt.md | 3 +++ .../moreunit/util/TestMethodDivinerJunit3PraefixTest.java | 7 +++++++ .../org/moreunit/util/TestMethodDivinerNoPraefixTest.java | 7 +++++++ test_diviner.sh | 1 + 4 files changed, 18 insertions(+) create mode 100644 test_diviner.sh diff --git a/.jules/bolt.md b/.jules/bolt.md index c299ba4c..b57dff27 100644 --- a/.jules/bolt.md +++ b/.jules/bolt.md @@ -30,3 +30,6 @@ ## 2026-05-22 - Replacing literal `replaceFirst` with manual string searches **Learning:** Using regex-based `replaceFirst` for literal search-and-replace, especially when dealing with prefixes or static replacements, is computationally expensive due to the overhead of `Pattern` compilation and matching. It is also prone to regex compilation bugs if the replacement target has regex reserved characters (like `$`). Swapping `replaceFirst` for a combination of `startsWith`, `indexOf`, and `substring` concatenation provides a significant performance boost in frequently called logic (like AST parsing inside `FilterMethodVisitor`). **Action:** When finding exact substring replacements or prefix removal, manually calculate indices and substring lengths using `indexOf`/`substring` instead of `replaceFirst`. +## 2026-05-22 - Code Coverage Failures after refactoring regex +**Learning:** When refactoring regex paths (like `replaceFirst`) into equivalent literal matches (`startsWith`, `indexOf`, `substring`), new specific edge-case branches (like `if (index != -1)`) might not have existing unit test coverage. This will trigger Codecov PR failures because the overall diff coverage drops below the required threshold. +**Action:** When making logical refactorings, proactively add unit tests for the new conditional branches introduced by the refactoring, even if they seem trivial, to ensure CI coverage checks pass. diff --git a/org.moreunit.test/test/org/moreunit/util/TestMethodDivinerJunit3PraefixTest.java b/org.moreunit.test/test/org/moreunit/util/TestMethodDivinerJunit3PraefixTest.java index 8a1daf6c..be1b5281 100644 --- a/org.moreunit.test/test/org/moreunit/util/TestMethodDivinerJunit3PraefixTest.java +++ b/org.moreunit.test/test/org/moreunit/util/TestMethodDivinerJunit3PraefixTest.java @@ -56,4 +56,11 @@ public void getTestMethodNameAfterRename() assertThat(testMethodDiviner.getTestMethodNameAfterRename("countMembers", "countAllMembers", "testCountMembersSpecialCase")).isEqualTo("testCountAllMembersSpecialCase"); assertThat(testMethodDiviner.getTestMethodNameAfterRename("countMembers", "countAllMembers", "testCountMembers")).isEqualTo("testCountAllMembers"); } + + @Test + public void getTestMethodNameAfterRename_noMatch() + { + TestMethodDiviner testMethodDiviner = new TestMethodDivinerJunit3Praefix(); + assertThat(testMethodDiviner.getTestMethodNameAfterRename("countMembers", "countAllMembers", "testSomethingElse")).isEqualTo("testSomethingElse"); + } } diff --git a/org.moreunit.test/test/org/moreunit/util/TestMethodDivinerNoPraefixTest.java b/org.moreunit.test/test/org/moreunit/util/TestMethodDivinerNoPraefixTest.java index 6ddd7d16..111e82c1 100644 --- a/org.moreunit.test/test/org/moreunit/util/TestMethodDivinerNoPraefixTest.java +++ b/org.moreunit.test/test/org/moreunit/util/TestMethodDivinerNoPraefixTest.java @@ -21,6 +21,13 @@ public void testGetTestMethodNameAfterRename() throws Exception assertThat(testMethodDivinerNoPraefix.getTestMethodNameAfterRename("getFoo", "get2Foo", "getFoo_getterWorks")).isEqualTo("get2Foo_getterWorks"); } + @Test + public void testGetTestMethodNameAfterRename_noMatch() throws Exception + { + TestMethodDivinerNoPraefix testMethodDivinerNoPraefix = new TestMethodDivinerNoPraefix(); + assertThat(testMethodDivinerNoPraefix.getTestMethodNameAfterRename("getFoo", "get2Foo", "somethingElse")).isEqualTo("somethingElse"); + } + @Test public void getMethodNameFromTestMethodName() throws Exception { diff --git a/test_diviner.sh b/test_diviner.sh new file mode 100644 index 00000000..068537c2 --- /dev/null +++ b/test_diviner.sh @@ -0,0 +1 @@ +# check line numbers for missing coverage