analysisScope toJson function by aakgna · Pull Request #1355 · wala/WALA

aakgna · 2024-01-04T00:52:59Z

Added a new function to serialize an AnalysisScope to JSON format.

msridhar

Overall looking good! I have a few comments. Also, we need to write a test for this new code. You can add it in the AnalysisScopeTest class: https://github.com/wala/WALA/blob/master/core/src/test/java/com/ibm/wala/core/tests/cha/AnalysisScopeTest.java#L21-L21 The test should create an AnalysisScope and then confirm the JSON generated by toJSON() matches what we expect

msridhar · 2024-01-24T23:58:47Z

core/src/main/java/com/ibm/wala/ipa/callgraph/AnalysisScope.java

+
+
+// import com.google.gson.Gson;


delete these lines

msridhar · 2024-01-24T23:59:54Z

core/src/main/java/com/ibm/wala/ipa/callgraph/AnalysisScope.java

  }

+  public String toJson() {
+    HashMap<String, Object> res = new HashMap<>();


For this code, let's used LinkedHashMap instead of HashMap to ensure a deterministic iteration order https://docs.oracle.com/javase/8/docs/api/java/util/LinkedHashMap.html

core/src/main/java/com/ibm/wala/ipa/callgraph/AnalysisScope.java

msridhar · 2024-01-25T00:01:56Z

core/src/test/java/com/ibm/wala/core/tests/callGraph/CallGraphTest.java

    };
  }
-}
+}


remove this change

Still not addressed

msridhar · 2024-01-25T00:02:04Z

core/src/test/java/com/ibm/wala/core/tests/cha/AnalysisScopeTest.java

        "found unexpected class");
  }
-}
+}


remove this change

msridhar · 2024-01-25T00:02:10Z

gradle/wrapper/gradle-wrapper.properties

 validateDistributionUrl=true
 zipStoreBase=GRADLE_USER_HOME
-zipStorePath=wrapper/dists
+zipStorePath=wrapper/dists


remove this change

msridhar · 2024-01-25T00:03:06Z

core/src/main/java/com/ibm/wala/ipa/callgraph/AnalysisScope.java

+    String[] exclusions = getExclusions().toString().split("\\|");
+    ArrayList<String> arr2 = new ArrayList<>();
+    for(int i = 0; i < exclusions.length; i++){
+      String word = exclusions[i];
+      word = word.replace("(", "");
+      word = word.replace(")", "");
+      arr2.add(word);
+    }


Cool, way to go the extra mile to parse the exclusions!

github-actions · 2024-01-30T00:08:33Z

Test Results

572 files ± 0 572 suites ±0 3h 37m 54s ⏱️ + 29m 35s
734 tests + 2 717 ✅ + 2 17 💤 ±0 0 ❌ ±0
3 554 runs +10 3 467 ✅ +10 87 💤 ±0 0 ❌ ±0

Results for commit efad143. ± Comparison against base commit 0e3068a.

♻️ This comment has been updated with latest results.

core/src/main/java/com/ibm/wala/ipa/callgraph/AnalysisScope.java

msridhar · 2024-01-31T19:39:05Z

core/src/test/java/com/ibm/wala/core/tests/callGraph/CallGraphTest.java

    };
  }
-}
+}


Still not addressed

msridhar · 2024-01-31T19:43:17Z

core/src/test/java/com/ibm/wala/core/tests/cha/AnalysisScopeTest.java

+                // new FileProvider().getFile("J2SEClassHierarchyExclusions.txt"),
+                new FileProvider().getFile("GUIExclusions.txt"),
+                AnalysisScopeTest.class.getClassLoader());
+        String exp = "{\"Loaders\":{\"Primordial\":[\"JarFileModule:/opt/homebrew/Cellar/openjdk/21.0.1/libexec/openjdk.jdk/Contents/Home/jmods/java.base.jmod\",\"Nested Jar File:primordial.jar.model\"],\"Extension\":[],\"Application\":[\"JarFileModule:/Users/aakgna/Documents/WALA-Research/WALA/core/build/resources/test/com.ibm.wala.core.testdata_1.0.0.jar\"],\"Synthetic\":[]},\"Exclusions\":[\"java\\\\/awt\\\\/.*\",\"javax\\\\/swing\\\\/.*\",\"sun\\\\/awt\\\\/.*\",\"sun\\\\/swing\\\\/.*\"]}";


This is looking better! But I expect this test will not pass in general, since we have absolute paths in it. So I guess a straightforward string comparison won't work. Here's an alternate approach:

Call scope.toJson() to get the JSON string

Re-parse this String using Gson

Check that specific parts of the resulting Map match what we expect

Can you try that? Let me know if it doesn't make sense

msridhar

Almost there! A couple more comments on the tests

msridhar · 2024-02-12T21:46:48Z

core/src/test/java/com/ibm/wala/core/tests/cha/AnalysisScopeTest.java

+            assertEquals(arr2, map.get("Exclusions"));
+        }
+        Type type2 = new TypeToken<LinkedHashMap<String, ArrayList<String>>>(){}.getType();
+        LinkedHashMap<String, ArrayList<String>> loaders = gson.fromJson(gson.toJson(map.get("Loaders")), type2);


Here I am confused, why do you need to serialize the data in the map back to JSON and then parse it again? I think you can get rid of this and just use some downcasts to get the data you want out of the map directly

msridhar · 2024-02-12T21:50:51Z

core/src/test/java/com/ibm/wala/core/tests/cha/AnalysisScopeTest.java

+        LinkedHashMap<String, Object> map = gson.fromJson(scope.toJson(), type);
+        System.out.println(map);
+        if(map.containsKey("Exclusions")) {
+            String[] exclusions = scope.getExclusions().toString().split("\\|");


Here we know what is in GUIExclusions.txt. So I think we can simplify this code. We can just do something like assertEquals(List.of("java\/awt\/.*", "javax\/swing\/.*", "sun\/awt\/.*", "sun\/swing\/.*"), map.get("Exclusions")). We can then get rid of this whole loop that re-parses the exclusions string

msridhar · 2024-02-12T21:55:30Z

core/src/test/java/com/ibm/wala/core/tests/cha/AnalysisScopeTest.java

+        }
+        Type type2 = new TypeToken<LinkedHashMap<String, ArrayList<String>>>(){}.getType();
+        LinkedHashMap<String, ArrayList<String>> loaders = gson.fromJson(gson.toJson(map.get("Loaders")), type2);
+        if(loaders.containsKey("Primordial")) {


Again we can simplify this code. We know the contents of wala.testdata.txt:

Primordial,Java,stdlib,base Primordial,Java,jarFile,primordial.jar.model Application,Java,jarFile,com.ibm.wala.core.testdata_1.0.0.jar

So I think we can write assertions like:

The keys of loaders should be exactly "Primordial","Application","Extension","Synthetic"

Extension and Synthetic should be mapped to empty lists

Primordial should be mapped to a list of length two. And one of the elements should contain `"primordial.jar.model"

Application should be mapped to a list of length 1, and that element should contain "com.ibm.wala.core.testdata_1.0.0.jar"

…t for AnalysisScope

msridhar

Some more comments

msridhar · 2024-02-28T02:07:54Z

core/src/main/java/com/ibm/wala/ipa/callgraph/AnalysisScope.java

+    LinkedHashMap<String, ArrayList<String>> loaders = new LinkedHashMap<>();
+    for (ClassLoaderReference loader : loadersByName.values()) {
+      ArrayList<String> arr = new ArrayList<>();
+


remove blank line

core/src/main/java/com/ibm/wala/ipa/callgraph/AnalysisScope.java

msridhar · 2024-02-28T02:10:39Z

core/src/test/java/com/ibm/wala/core/tests/cha/AnalysisScopeTest.java

+    AnalysisScope tempScope =
+        AnalysisScopeReader.instance.readJavaScope(
+            TestConstants.WALA_TESTDATA,
+            new FileProvider().getFile("GUIExclusions.txt"),
+            AnalysisScopeTest.class.getClassLoader());


We don't need to create this temp scope just to test the exclusions. Just skip testing the exclusions here

msridhar · 2024-02-28T02:16:48Z

core/src/test/java/com/ibm/wala/core/tests/cha/AnalysisScopeTest.java

+    scope.setExclusions(tempScope.getExclusions());
+    String[] stdlibs = WalaProperties.getJ2SEJarFiles();
+    Arrays.sort(stdlibs);
+    int cnt = 0;


Add a comment explaining why you are stopping at 5. Also the value 5 should be stored in a local variable and then that variable should be used in multiple places

But this code would be simpler if you just got rid of the cnt variable and added everything. Is there any reason to add the limit?

msridhar · 2024-02-28T02:18:15Z

core/src/test/java/com/ibm/wala/core/tests/cha/AnalysisScopeTest.java

+    assertThat(
+        loaders.get("Primordial"),
+        hasItem(
+            "JarFileModule:/Users/aakgna/Library/Java/JavaVirtualMachines/corretto-11.0.15/Contents/Home/jmods/java.base.jmod"));


This check will not pass on other machines. You have the stdlibs array above. You should use the strings there (which give the full paths to the jars where the tests are running) to check that the right strings are in the JSON.

msridhar · 2024-02-28T18:02:28Z

core/src/test/java/com/ibm/wala/core/tests/cha/AnalysisScopeTest.java

+            .get("Primordial")
+            .get(0)
+            .contains(
+                "Java/JavaVirtualMachines/corretto-11.0.15/Contents/Home/jmods/java.base.jmod"));


This does not fully address my comment. Please take the paths directly from the stdlibs array and check that they match.

msridhar · 2024-03-04T20:17:39Z

core/src/test/java/com/ibm/wala/core/tests/cha/AnalysisScopeTest.java

+        new HashSet<>(List.of("Primordial", "Extension", "Application", "Synthetic"));
+    assertEquals(loaders.keySet(), loaderKeys);
+    assertEquals(stdlibs.length, loaders.get("Primordial").size());
+    assertTrue(loaders.get("Primordial").get(0).contains("/Contents/Home/jmods/java.base.jmod"));


This is still wrong, and tests are still failing. You need to take the relevant strings from the stdlibs array for this comparison, rather than just doing get(0) and comparing to a constant string.

aakgna added 8 commits November 19, 2023 17:14

added toJson function to AnalysisScope

1187f33

added one successful test for toJson in Analysis Scope

3a90899

added json capability

6c81529

implemented toJson functionality

163aa8e

returned right value

6f69ce9

added changes to toJson

307607a

returned correct value

cb5dd7b

changes .settings

b489343

msridhar self-requested a review January 22, 2024 20:42

aakgna and others added 2 commits January 22, 2024 16:59

removed unnecessary changes

a38c5b7

Merge branch 'wala:master' into master

ab16227

msridhar reviewed Jan 25, 2024

View reviewed changes

aakgna and others added 3 commits January 30, 2024 11:29

Merge branch 'wala:master' into master

a43b836

added linkedHashmap to AnalysisScope and created test for toJson

6d15461

deleted unnecessary lines

e57b590

msridhar reviewed Jan 31, 2024

View reviewed changes

aakgna and others added 2 commits February 10, 2024 16:26

Merge branch 'wala:master' into master

0f8f978

implemented changes for toJson and added endlines

c237c76

msridhar reviewed Feb 12, 2024

View reviewed changes

aakgna and others added 6 commits February 13, 2024 11:27

completed changes for AnalysisScopeTest and removed 1 import statemen…

fbe9c59

…t for AnalysisScope

Tune up test and fix formatting

3cfff82

suppress a warning

ebd73ae

added custom analysisScope test

89359aa

sorted stdlib

ed5500c

formatting

7036d90

msridhar reviewed Feb 28, 2024

View reviewed changes

updated toJson custom test and added onto toJson method

9b7b7e9

msridhar reviewed Feb 28, 2024

View reviewed changes

test out path for stdlibs

a41401a

aakgna and others added 4 commits March 3, 2024 12:11

Merge branch 'master' into master

c7707eb

modified file path to match all machines

8b9c04d

formatting

15a4a1e

Merge branch 'master' into aakgna/master

7729aa7

msridhar reviewed Mar 4, 2024

View reviewed changes

aakgna added 2 commits March 4, 2024 13:52

changed assertion in testToJsonCustom

f777f66

added for loop in testToJsonCustom (stdlibs)

efad143

msridhar approved these changes Mar 7, 2024

View reviewed changes

msridhar merged commit ae3b95d into wala:master Mar 7, 2024

                   };
                 }
-              }
+              }

                
                    No newline at end of file

Conversation

aakgna commented Jan 4, 2024 • edited by msridhar 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

github-actions bot commented Jan 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Test Results

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

msridhar left a comment

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!

msridhar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

aakgna commented Jan 4, 2024 •

edited by msridhar

Loading

github-actions bot commented Jan 30, 2024 •

edited

Loading