refactor: remove getTestName from IRTests.java so tests do not rely on method name #1485

jkhaliqi · 2025-03-06T21:05:11Z

Cleaned up ECJJava15IRTest.java by refactoring the tests to be parameterized to not rely on method name for running the tests. Also removed the getTestName from IRTests so the tests do not rely on that method to get the name of the test it should run. With this we pass in the name and tests can now be parameterized as well instead of relying on naming tests differently

github-actions · 2025-03-06T21:13:45Z

Test Results

827 files ± 0 827 suites ±0 4h 20m 54s ⏱️ - 21m 45s
866 tests +101 849 ✅ +101 17 💤 ±0 0 ❌ ±0
5 239 runs ± 0 5 118 ✅ ± 0 121 💤 ±0 0 ❌ ±0

Results for commit e04fcf9. ± Comparison against base commit 4722084.

This pull request removes 59 and adds 160 tests. Note that renamed tests count towards both.

com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ testAnnotations()
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ testAnonGeneNullarySimple()
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ testAnonymousGenerics()
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ testBasicsGenerics()
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ testCocovariant()
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ testCustomGenericsAndFields()
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ testEnumSwitch()
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ testExplicitBoxingTest()
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ testGenericArrays()
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ testGenericMemberClasses()
…

com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ [10] java15IRTestName=GenericMemberClasses
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ [11] java15IRTestName=GenericSuperSink
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ [12] java15IRTestName=MoreOverriddenGenerics
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ [13] java15IRTestName=NotSoSimpleEnums
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ [14] java15IRTestName=OverridesOnePointFour
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ [15] java15IRTestName=SimpleEnums
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ [16] java15IRTestName=SimpleEnums2
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ [17] java15IRTestName=TypeInferencePrimAndStringOp
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ [18] java15IRTestName=Varargs
com.ibm.wala.cast.java.test.ECJJava15IRTest ‑ [19] java15IRTestName=VarargsCovariant
…

♻️ This comment has been updated with latest results.

jkhaliqi · 2025-03-06T21:37:53Z

@msridhar @juliandolby Hey whenever you get a chance could I get review on this, thank you!

msridhar · 2025-03-06T21:45:27Z

@jkhaliqi I love it! Do you have time to take this further? This is the key function that pulls out the name of the currently-running test:

WALA/cast/java/src/testFixtures/java/com/ibm/wala/cast/java/test/IRTests.java

Lines 357 to 366 in 90350c7

    
           protected String getTestName() { 
        
             StackTraceElement stack[] = new Throwable().getStackTrace(); 
        
             for (int i = 0; i <= stack.length; i++) { 
        
               if (stack[i].getMethodName().startsWith("test")) { 
        
                 return stack[i].getMethodName(); 
        
               } 
        
             } 
        
             throw new Error("test method not found"); 
        
           }

I'd love it if we could push this cleanup further so that the function above could just be deleted and everywhere we rely on it we instead use parameterized tests. I think it would involve a few more changes similar to what you've already done and (hopefully!) deleting a bunch of code. What do you think?

jkhaliqi · 2025-03-06T22:02:29Z

Hey sounds good, and thank you! I'll give that a try and see how far I get

cast/java/ecj/src/test/java/com/ibm/wala/cast/java/test/ECJJava15IRTest.java

msridhar · 2025-03-07T17:41:23Z

@jkhaliqi is this ready for review?

jkhaliqi · 2025-03-07T17:43:40Z

@msridhar Yup, I believe this is ready for review, whenever you get a chance, thank you!

jkhaliqi · 2025-03-07T18:06:07Z

I just changed the JavaIRTests to be alphabetical as well for parametrized tests, but all should be good now!

msridhar

Looking great! A couple of comments particularly on JavaIRTests

msridhar · 2025-03-08T20:57:30Z

cast/java/src/testFixtures/java/com/ibm/wala/cast/java/test/JavaIRTests.java

-        true,
-        null);
-  }
+  static List<? extends IRAssertion> caForInterfaceTest1 =


This naming scheme caForXXX is confusing. What does ca mean? Can we use something more descriptive?

I wasn't exactly sure what ca also meant I got this variable name from the fourth parameter inside of runTests. But i'm assuming it has something to do with the callGraph and just changed it to be callAssertionsForXXX. Let me know if theres a better name.

msridhar · 2025-03-08T20:59:18Z

cast/java/src/testFixtures/java/com/ibm/wala/cast/java/test/JavaIRTests.java

  }

  @Test
-  public void testArrayLiteral2() throws IllegalArgumentException, CancelException, IOException {
+  public void testTwoClasses() throws IllegalArgumentException, CancelException, IOException {


Why are the remaining tests still here and not just run via the parameterized tests?

Some of the remaining tests call runTests and store its output in a variable and then do other stuff with it so I just left those alone.

msridhar · 2025-03-10T18:30:13Z

@jkhaliqi can you just add commits to your branch from now on instead of force pushing? That will make it easier to see what is changing. I will squash the commits before merging.

jkhaliqi · 2025-03-10T20:10:37Z

@msridhar Sounds good, sorry about that!

msridhar

Thanks again so much for this cleanup! One more comment / question

msridhar · 2025-03-12T15:07:23Z

cast/java/src/testFixtures/java/com/ibm/wala/cast/java/test/JavaIRTests.java

+        runTest(
+            singleTestSrc("InnerClassA"),
+            rtJar,
+            simpleTestEntryPoint("InnerClassA"),
+            new ArrayList<>(),
+            true,
+            null);


There are many call sites now where we pass exactly these arguments, except that the test name is different. Could we possibly add a new wrapper method for runTest that takes a single String argument and then calls runTest appropriately, and then replace all these other calls with a call to the wrapper method?

Added in the wrapper, thank you!

msridhar · 2025-03-13T01:35:36Z

Thanks again for the contribution!

msridhar · 2025-03-13T01:36:01Z

@liblit I think you will need to approve again before this can land

liblit

According to this bot comment, this change results in a net increase of 101 tests. Is that expected?

jkhaliqi · 2025-03-13T18:23:51Z

@liblit I believe so, is there a way I can double check all the test cases the bot only shows a couple before showing the ...

msridhar · 2025-03-16T23:58:48Z

@liblit I believe so, is there a way I can double check all the test cases the bot only shows a couple before showing the ...

Yeah, this is pretty hard to check based on the bot comment which cuts off the output. Any ideas @liblit? I can see various artifacts related to the executed tests at the bottom of the page for the action run but not clear how to get the full differences from that.

liblit · 2025-03-17T02:15:32Z

You could run the tests on your local machine. Test before and after your proposed changes. Presumably each of those will create some kind of artifact you can compare without truncation.

msridhar · 2025-03-17T17:34:50Z

You could run the tests on your local machine. Test before and after your proposed changes. Presumably each of those will create some kind of artifact you can compare without truncation.

Sure, I guess I was wondering if there's an easy way to run whatever code generates the comments on the pull requests, though I guess one could just run diff on the relevant files.

msridhar · 2025-03-17T18:38:05Z

In 0db3fc9 I tried bumping up the limit on how many test changes are listed in the comment, using the test_changes_limit setting, but it didn't seem to work 😐

jkhaliqi · 2025-03-21T00:47:05Z

@liblit Can you explain more on the test and artifact comparison I wasn't able to follow or find the artifacts created after running the test. But I did just run and compare and everything is the same as far as these tests with first number being current change and second one before changes. There's only an increase in one for the tests because the @parameterized part will run but everything else ran the same...
java15irtest -> 22 -> 21
java17 -> 7 -> 6
testcomments-> same
Issue666Test.java -> same
Issue667Test -> same
jlextest -> same
javairtest -> 37-> 37
syncduplicator -> same

msridhar · 2025-03-22T00:08:32Z

Thanks again for the contribution @jkhaliqi!

…n method name (wala#1485) Cleaned up `ECJJava15IRTest.java` by refactoring the tests to be parameterized to not rely on method name for running the tests. Also removed the `getTestName` from `IRTests` so the tests do not rely on that method to get the name of the test it should run. With this we pass in the name and tests can now be parameterized as well instead of relying on naming tests differently --------- Co-authored-by: Manu Sridharan <msridhar@gmail.com>

liblit requested changes Mar 6, 2025

View reviewed changes

cast/java/ecj/src/test/java/com/ibm/wala/cast/java/test/ECJJava15IRTest.java Outdated Show resolved Hide resolved

jkhaliqi added 2 commits March 6, 2025 15:05

refactored ECJJava15IRTests to be parameterized

cd21a41

refactored ECJJava17IRTests to be parameterized

5668e84

jkhaliqi force-pushed the parameterize-IR branch 5 times, most recently from d1483aa to ffbd13b Compare March 7, 2025 00:58

jkhaliqi changed the title ~~refactored ECJJava15IRTests to be parameterized~~ refactor: remove getTestName from IRTests.java so tests do noot rely on method name Mar 7, 2025

jkhaliqi changed the title ~~refactor: remove getTestName from IRTests.java so tests do noot rely on method name~~ refactor: remove getTestName from IRTests.java so tests do not rely on method name Mar 7, 2025

jkhaliqi force-pushed the parameterize-IR branch from ffbd13b to 2eb871e Compare March 7, 2025 18:05

msridhar reviewed Mar 8, 2025

View reviewed changes

jkhaliqi force-pushed the parameterize-IR branch from 2eb871e to 853fdb6 Compare March 10, 2025 17:51

refactor: remove the getTestName method in IRTests

520e80e

jkhaliqi force-pushed the parameterize-IR branch from 853fdb6 to 520e80e Compare March 10, 2025 17:55

msridhar reviewed Mar 12, 2025

View reviewed changes

added runTest wrapper for common call

e91d747

jkhaliqi force-pushed the parameterize-IR branch from 0927112 to e91d747 Compare March 12, 2025 23:09

msridhar previously approved these changes Mar 13, 2025

View reviewed changes

msridhar requested a review from liblit March 13, 2025 01:35

msridhar enabled auto-merge (squash) March 13, 2025 01:35

liblit requested changes Mar 13, 2025

View reviewed changes

used runTest wrapper for testComments

e8ed601

auto-merge was automatically disabled March 13, 2025 18:21
Head branch was pushed to by a user without write access

jkhaliqi dismissed msridhar’s stale review via e8ed601 March 13, 2025 18:22

msridhar added 2 commits March 17, 2025 10:56

try to bump limit on reported test changes

0db3fc9

Merge branch 'master' into parameterize-IR

e04fcf9

Merge branch 'master' into parameterize-IR

a58933a

liblit approved these changes Mar 22, 2025

View reviewed changes

msridhar enabled auto-merge (squash) March 22, 2025 00:08

msridhar merged commit 0080591 into wala:master Mar 22, 2025
10 checks passed

refactor: remove getTestName from IRTests.java so tests do not rely on method name #1485

refactor: remove getTestName from IRTests.java so tests do not rely on method name #1485

Uh oh!

Conversation

jkhaliqi commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

jkhaliqi commented Mar 6, 2025

Uh oh!

msridhar commented Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jkhaliqi commented Mar 6, 2025

Uh oh!

Uh oh!

msridhar commented Mar 7, 2025

Uh oh!

jkhaliqi commented Mar 7, 2025

Uh oh!

jkhaliqi commented Mar 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

msridhar left a comment

Choose a reason for hiding this comment

Uh oh!

msridhar Mar 8, 2025

Choose a reason for hiding this comment

Uh oh!

jkhaliqi Mar 10, 2025 • 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!

msridhar Mar 8, 2025

Choose a reason for hiding this comment

Uh oh!

jkhaliqi Mar 10, 2025

Choose a reason for hiding this comment

Uh oh!

msridhar commented Mar 10, 2025

Uh oh!

jkhaliqi commented Mar 10, 2025

Uh oh!

msridhar left a comment

Choose a reason for hiding this comment

Uh oh!

msridhar Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

jkhaliqi Mar 12, 2025

Choose a reason for hiding this comment

Uh oh!

msridhar commented Mar 13, 2025

Uh oh!

msridhar commented Mar 13, 2025

Uh oh!

liblit left a comment

Choose a reason for hiding this comment

Uh oh!

jkhaliqi commented Mar 13, 2025

Uh oh!

msridhar commented Mar 16, 2025

Uh oh!

liblit commented Mar 17, 2025

Uh oh!

msridhar commented Mar 17, 2025

Uh oh!

msridhar commented Mar 17, 2025

Uh oh!

jkhaliqi commented Mar 21, 2025

Uh oh!

msridhar commented Mar 22, 2025

Uh oh!

Uh oh!

Uh oh!

jkhaliqi commented Mar 6, 2025 •

edited

Loading

github-actions bot commented Mar 6, 2025 •

edited

Loading

msridhar commented Mar 6, 2025 •

edited

Loading

jkhaliqi commented Mar 7, 2025 •

edited

Loading

jkhaliqi Mar 10, 2025 •

edited

Loading