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 29m 15s ⏱️ - 22m 14s
866 tests +101 849 ✅ +101 17 💤 ±0 0 ❌ ±0
5 239 runs ± 0 5 118 ✅ ± 0 121 💤 ±0 0 ❌ ±0

Results for commit 520e80e. ± Comparison against base commit 90350c7.

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

liblit · 2025-03-06T22:19:45Z

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

+        "VarargsOverriding",
+        "EnumSwitch",
+        "OverridesOnePointFour",
+        "VarargsCovariant",
+        "SimpleEnums",
+        "GenericSuperSink",
+        "AnonGeneNullarySimple",
+        "AnonymousGenerics",
+        "NotSoSimpleEnums",
+        "Varargs",
+        "GenericArrays",
+        "Annotations",
+        "Cocovariant",
+        "ExplicitBoxingTest",
+        "Wildcards",
+        "BasicsGenerics",
+        "CustomGenericsAndFields",
+        "TypeInferencePrimAndStringOp",
+        "SimpleEnums2",
+        "GenericMemberClasses",
+        "MoreOverriddenGenerics");


If the order of these items does not matter, then please sort them alphabetically. That will make this list easier to scan when adding new items, easier to avoid adding duplicates, etc..

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!

liblit requested changes Mar 6, 2025

View reviewed changes

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

jkhaliqi commented Mar 6, 2025 •

edited

Loading

github-actions bot commented Mar 6, 2025 •

edited

Loading

jkhaliqi commented Mar 6, 2025

msridhar commented Mar 6, 2025 •

edited

Loading

jkhaliqi commented Mar 6, 2025

liblit Mar 6, 2025

msridhar commented Mar 7, 2025

jkhaliqi commented Mar 7, 2025

jkhaliqi commented Mar 7, 2025 •

edited

Loading

msridhar left a comment

msridhar Mar 8, 2025

jkhaliqi Mar 10, 2025 •

edited

Loading

msridhar Mar 8, 2025

jkhaliqi Mar 10, 2025

msridhar commented Mar 10, 2025

jkhaliqi commented Mar 10, 2025

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

Are you sure you want to change the base?

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

Conversation

jkhaliqi commented Mar 6, 2025 • edited Loading

github-actions bot commented Mar 6, 2025 • edited Loading

Test Results

jkhaliqi commented Mar 6, 2025

msridhar commented Mar 6, 2025 • edited Loading

jkhaliqi commented Mar 6, 2025

liblit Mar 6, 2025

Choose a reason for hiding this comment

msridhar commented Mar 7, 2025

jkhaliqi commented Mar 7, 2025

jkhaliqi commented Mar 7, 2025 • edited Loading

msridhar left a comment

Choose a reason for hiding this comment

msridhar Mar 8, 2025

Choose a reason for hiding this comment

jkhaliqi Mar 10, 2025 • edited Loading

Choose a reason for hiding this comment

msridhar Mar 8, 2025

Choose a reason for hiding this comment

jkhaliqi Mar 10, 2025

Choose a reason for hiding this comment

msridhar commented Mar 10, 2025

jkhaliqi commented Mar 10, 2025

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