Compile and overwrite test #175

judovana · 2021-10-07T15:09:02Z

No description provided.

Where SourceTestClassWrapper is abstract, and do all ahrd work. The TestingDummyHelper jsut sets pkg, classname and class body

CliTest.java and CompileUploadCliTest are now extending AbstractAgentNeedingTest.java and so shoudl have no shared duplicated code. CompileUploadCliTest is now partially similar to CliTest, but is using ModifiableDummyTestingHelper instead of TestingDummyHelper

Oh it did usefull thinhg! It removed all nasty static imports!

AurumTheEnd · 2021-10-08T08:22:59Z

I would suggest moving the "utility" classes to a different package in order to differentiate them from the actual org.jrd.backend.data test classes (CliTest, CompileUploadCliTest, but also ArchiveManagerOptionsTest). Thoughts?

judovana · 2021-10-08T08:59:12Z

not much agaist. What is the "AbstractAgentNeedingTest" then :)

…

-- Mgr. Jiri Vanek ***@***.***

---------- Původní e-mail ---------- Od: Prokop Tunel ***@***.***> Komu: pmikova/java-runtime-decompiler ***@***.*** github.com> Datum: 8. 10. 2021 10:23:18 Předmět: Re: [pmikova/java-runtime-decompiler] Compile and overwrite test (# 175) " I would suggest moving the "utility" classes to a different package in order to differentiate them from the actual org.jrd.backend.data test classes ( CliTest, CompileUploadCliTest, but also ArchiveManagerOptionsTest). Thoughts? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub (#175 (comment)) , or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAWFCS2PCEUSINNVNHB25M3UF2S67ANCNFSM5FRQV6GQ) . Triage notifications on the go with GitHub Mobile for iOS (https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or Android (https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub) . "

had to extract few constants on the fly

AurumTheEnd · 2021-10-08T17:11:15Z

Tests will fail until #111 is implemented. CompileUploadCliTest is currently essentially dependent on what plugins the user set up with the GUI, which is definitely unwanted testing behavior.

EDIT: it is too late for me to understand why the CI test passed. No idea. Dumbfounded. Perplexed. Astonished. Too many emotions.

EDIT EDIT: Oh yeah, in Model, we are creating PluginManager, which on fresh installs imports all the plugins because of ee08f01. But because I don't have a fresh install, but am also missing some plugins because I don't use them, their tests fail locally. Very interesting and unintuitive behavior.

AurumTheEnd · 2021-10-08T17:23:52Z

The Windows test has been running for 10 minutes, that isn't good. Mainly because I will have to be the one fixing it.

judovana · 2021-10-08T18:04:49Z

Tests will fail until #111 is implemented. CompileUploadCliTest is currently essentially dependent on what plugins the user set up with the GUI, which is definitely unwanted testing behavior.

Right you are. I will add some assume.
Btw, do you mind to check my usage of streams.get.... ?
In the api test, the output of compilation is never here.

EDIT: it is too late for me to understand why the CI test passed. No idea. Dumbfounded. Perplexed. Astonished. Too many emotions.

yah:(
I'm inclining to the bad unloading of agent.

EDIT EDIT: Oh yeah, in Model, we are creating PluginManager, which on fresh installs imports all the plugins because of ee08f01. But because I don't have a fresh install, but am also missing some plugins because I don't use them, their tests fail locally. Very interesting and unintuitive behavior.

Very interesting!

That was also my impression, that the plugin manager shoudl work with fresh install. And I have correct setup :)

AurumTheEnd · 2021-10-08T19:56:33Z

Btw, do you mind to check my usage of streams.get.... ?

I have not done a in-depth review yet. When I do in the next few days, I'll comment on it. Please don't merge this PR until then :D

EDIT: it is too late for me to understand why the CI test passed.

I'm inclining to the bad unloading of agent.

I was talking about the CompileAndUpload tests regarding different plugins. The passing overwrite test is a separate mystery.

…cli and args For Prokop: fields - class instance variales - are here to keep state of the object. Local variables, are here to see state of method. All this allows you better separation of member methods, utility methods and future exnesions without massive refactoings

…ime-decompiler into comileAndOverwriteTest

judovana · 2021-10-09T06:55:47Z

.github/workflows/maven.yaml

@@ -76,4 +76,4 @@ jobs:
      - name: Test Cli
        run: |
          cd jrd
-          mvn --batch-mode test -Dtest=CliTest -DfailIfNoTests=false
+          mvn --batch-mode test -Dtest=*CliTest -DfailIfNoTests=false


Why there is this condition at all? Test all!
Also the -DfailIfNoTests=false seems wrong. lets fail

AurumTheEnd · 2021-10-09T18:38:34Z

I see your 20 minute timeout still didn't help on Windows. Since all tests finish in under a minute on Ubuntu, this is a more serious issue than Windows just being slightly slower (which was the case prior to this PR - Windows build took about 1.5x the total time of the Ubuntu one).

judovana · 2021-10-10T08:03:23Z

I see your 20 minute timeout still didn't help on Windows. Since all tests finish in under a minute on Ubuntu, this is a more serious issue than Windows just being slightly slower (which was the case prior to this PR - Windows build took about 1.5x the total time of the Ubuntu one).

yy. remopved

runtime-decompiler/src/test/java/org/jrd/backend/data/AbstractSourceTestClass.java

runtime-decompiler/src/test/java/org/jrd/backend/data/CliTest.java

AurumTheEnd · 2021-10-11T14:04:33Z

runtime-decompiler/src/test/java/org/jrd/backend/data/CompileUploadCliTest.java

+    void testBytes() throws Exception {
+        testBytes(dummy.getPid());
+        testBytes(dummy.getClasspath());
+    }
+
+    void testBase64Bytes(String pucComponent) throws Exception {
+        args = new String[]{BASE64, pucComponent, dummy.getClassRegex()};
+        cli = new Cli(args, model);
+
+        cli.consumeCli();
+
+        byte[] base64Bytes = streams.getOut().trim().getBytes(StandardCharsets.UTF_8);
+        byte[] fileContents = Files.readAllBytes(Path.of(dummy.getDotClassPath()));
+        byte[] encoded = Base64.getEncoder().encode(fileContents);
+
+        assertArrayEquals(encoded, base64Bytes);
+    }
+
+    @Test
+    void testBase64Bytes() throws Exception {
+        testBase64Bytes(dummy.getPid());
+        testBase64Bytes(dummy.getClasspath());
+    }
+
+    void testBytesAndBase64BytesEqual(String pucComponent) throws Exception {
+        args = new String[]{BYTES, pucComponent, dummy.getClassRegex()};
+        cli = new Cli(args, model);
+
+        cli.consumeCli();
+        byte[] bytes = streams.getOutBytes();
+
+        args = new String[]{BASE64, pucComponent, dummy.getClassRegex()};
+        cli = new Cli(args, model);
+
+        cli.consumeCli();
+        String base64 = streams.getOut().trim();
+        byte[] decoded = Base64.getDecoder().decode(base64);
+
+        assertArrayEquals(bytes, decoded);
+    }
+
+    @Test
+    void testBytesAndBase64BytesEqual() throws Exception {
+        testBytesAndBase64BytesEqual(dummy.getPid());
+        testBytesAndBase64BytesEqual(dummy.getClasspath());
+    }
+
+
+    void testDecompileJavap(String pucComponent, String option) throws Exception {
+        args = new String[]{DECOMPILE, pucComponent, "javap" + option, dummy.getClassRegex()};
+        cli = new Cli(args, model);
+
+        cli.consumeCli();
+        String jrdDisassembled = streams.getOut();
+        String javapDisassembled = dummy.executeJavaP(option);
+
+        // JRD javap has additional debug comment lines + header is different
+        assertEqualsWithTolerance(jrdDisassembled, javapDisassembled, 0.8);
+    }
+
+    @ParameterizedTest
+    @ValueSource(strings = {"", "-v"})
+    void testDecompileJavap(String option) throws Exception {
+        testDecompileJavap(dummy.getPid(), option);
+        testDecompileJavap(dummy.getClasspath(), option);
+    }


There should be and explanatory comment of why these tests are duplicated from the normal CliTest here: They support the later tests where bytes and javap functionality is needed, but not the tested subject.

AurumTheEnd · 2021-10-11T14:06:33Z

runtime-decompiler/src/test/java/org/jrd/backend/data/AbstractAgentNeedingTest.java

@@ -144,14 +144,15 @@ private static int min3(int a, int b, int c) {
        }
    }

-    public static class StreamWrappers {
+
+    public static class JunitStderrOutThief {


I do not understand this renaming.

AurumTheEnd · 2021-10-11T14:13:19Z

runtime-decompiler/src/test/java/org/jrd/backend/data/CliTest.java

+     * Although the class definition was transforemd, in OpenJDK HotSpot
+     * this implementation of dummy do not request the new definition,
+     * adn continues to spit out its jitted output


Interesting. So it's basically the exact opposite of what we originally expected to happen: source code changes (which we expected not to withstand multiple cli calls), but output doesn't change (which we expected to change).

AurumTheEnd · 2021-10-11T14:15:48Z

runtime-decompiler/src/test/java/org/jrd/backend/data/AbstractAgentNeedingTest.java

+        }
+    }
+
+    public static String readBinaryAsString(FileInputStream input, String charBase, CodingErrorAction action) throws IOException {


Unused parameter action.

AurumTheEnd · 2021-10-11T15:01:14Z

runtime-decompiler/src/main/java/org/jrd/backend/data/Cli.java

@@ -262,13 +268,28 @@ private void overwrite() throws Exception {
        }
    }

+    @SuppressFBWarnings(value = "OS_OPEN_STREAM", justification = "The stream is clsoed as conditionally as is created")


Probably could be cheesed by having the condition as a final boolean variable. Probably.

judovana · 2021-10-14T17:59:09Z

I see your 20 minute timeout still didn't help on Windows. Since all tests finish in under a minute on Ubuntu, this is a more serious issue than Windows just being slightly slower (which was the case prior to this PR - Windows build took about 1.5x the total time of the Ubuntu one).

yy. remopved

see #182

Please fix in a separate PR.

AurumTheEnd · 2021-10-14T20:51:40Z

Please fix in a separate PR.

Nah. :)

judovana · 2021-10-15T06:47:07Z

yah!

…

-- Mgr. Jiri Vanek ***@***.***

---------- Původní e-mail ---------- Od: Prokop Tunel ***@***.***> Komu: pmikova/java-runtime-decompiler ***@***.*** github.com> Datum: 14. 10. 2021 22:51:56 Předmět: Re: [pmikova/java-runtime-decompiler] Compile and overwrite test (# 175) " " Please fix in a separate PR. " Nah. :) — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub (#175 (comment)) , or unsubscribe (https://github.com/notifications/unsubscribe-auth/AAWFCS6H2QCNJYEWHP7GY4TUG47GNANCNFSM5FRQV6GQ) . Triage notifications on the go with GitHub Mobile for iOS (https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675) or Android (https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub) . "

… issues Which is my only guess for why "javap -v" was stuck on loop forever. Stdout & stderr are now read in a loop rather than at once, and also their reading occurs at least once before the process is waitedFor().

The switch has to be in quotes, because Powershell, the default runner on windows-latest, interprets the dot in the property name as... some sort of keyword.

Instead of using some word-boundary using regex to match the plugin, we can skip the whole CLI and temp file ordeal by using the plugin manager.

judovana added 2 commits October 7, 2021 16:51

Refactored TestingDummyHelper to it and SourceTestClassWrapper

17784d3

Where SourceTestClassWrapper is abstract, and do all ahrd work. The TestingDummyHelper jsut sets pkg, classname and class body

AurumTheEnd changed the title ~~Comile and overwrite test~~ Compile and overwrite test Oct 7, 2021

Made checkstyle happy

7a63bcd

Oh it did usefull thinhg! It removed all nasty static imports!

Added checks about modifed/same stdout of running test class

4442dde

judovana and others added 3 commits October 8, 2021 16:44

Added tests for compilation

a78837d

had to extract few constants on the fly

Added test to test api

ff68b6a

Made GitHub Ci run all tests matching *CliTest

1b5c069

Introduced a 10 minute timeout into CI "Test Cli" step

7861716

judovana added 4 commits October 9, 2021 07:54

listjvms, listplugins, api now can use saveaas

799b770

Plugin dependent tests are now ignored if given plugin is not configured

8202359

Merge branch 'comileAndOverwriteTest' of github.com:pmikova/java-runt…

7f2058d

…ime-decompiler into comileAndOverwriteTest

judovana commented Oct 9, 2021

View reviewed changes

judovana mentioned this pull request Oct 9, 2021

revisit github actions test conditions #178

Closed

Removed duplicated code in CompileUploadCliTest

fdbdcf5

judovana force-pushed the comileAndOverwriteTest branch from 7e3e241 to fdbdcf5 Compare October 10, 2021 08:00

AurumTheEnd approved these changes Oct 11, 2021

View reviewed changes

judovana mentioned this pull request Oct 11, 2021

introduction of testprocess output capture stuck windows test machine #182

Closed

AurumTheEnd force-pushed the comileAndOverwriteTest branch 2 times, most recently from bc5fcdb to 5d6e13e Compare October 14, 2021 20:32

AurumTheEnd force-pushed the comileAndOverwriteTest branch from 5d6e13e to ff36371 Compare October 14, 2021 20:35

AurumTheEnd force-pushed the comileAndOverwriteTest branch 2 times, most recently from 60ded2b to 0500b20 Compare October 14, 2021 21:18

AurumTheEnd force-pushed the comileAndOverwriteTest branch from 0500b20 to 17459ed Compare October 15, 2021 09:18

AurumTheEnd added 3 commits October 15, 2021 11:22

Made GitHub CI CliTests show what's being skipped

9ec2f10

The switch has to be in quotes, because Powershell, the default runner on windows-latest, interprets the dot in the property name as... some sort of keyword.

Fixed CliTest of first plugin in list not running

036a98b

Instead of using some word-boundary using regex to match the plugin, we can skip the whole CLI and temp file ordeal by using the plugin manager.

AurumTheEnd force-pushed the comileAndOverwriteTest branch from 17459ed to 036a98b Compare October 15, 2021 09:23

AurumTheEnd linked an issue Oct 15, 2021 that may be closed by this pull request

javap exec is missuning shared stderr/out #189

Closed

AurumTheEnd merged commit ea133d5 into master Oct 15, 2021

judovana deleted the comileAndOverwriteTest branch February 22, 2024 11:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Compile and overwrite test #175

Compile and overwrite test #175

judovana commented Oct 7, 2021

AurumTheEnd commented Oct 8, 2021 •

edited

Loading

judovana commented Oct 8, 2021 via email

AurumTheEnd commented Oct 8, 2021 •

edited

Loading

AurumTheEnd commented Oct 8, 2021

judovana commented Oct 8, 2021

AurumTheEnd commented Oct 8, 2021

judovana Oct 9, 2021

judovana Oct 10, 2021

AurumTheEnd commented Oct 9, 2021

judovana commented Oct 10, 2021

AurumTheEnd Oct 11, 2021

AurumTheEnd Oct 11, 2021

AurumTheEnd Oct 11, 2021

AurumTheEnd Oct 11, 2021

AurumTheEnd Oct 11, 2021

judovana commented Oct 14, 2021 •

edited by AurumTheEnd

Loading

AurumTheEnd commented Oct 14, 2021

judovana commented Oct 15, 2021 via email

Compile and overwrite test #175

Compile and overwrite test #175

Conversation

judovana commented Oct 7, 2021

AurumTheEnd commented Oct 8, 2021 • edited Loading

judovana commented Oct 8, 2021 via email

AurumTheEnd commented Oct 8, 2021 • edited Loading

AurumTheEnd commented Oct 8, 2021

judovana commented Oct 8, 2021

AurumTheEnd commented Oct 8, 2021

judovana Oct 9, 2021

Choose a reason for hiding this comment

judovana Oct 10, 2021

Choose a reason for hiding this comment

AurumTheEnd commented Oct 9, 2021

judovana commented Oct 10, 2021

AurumTheEnd Oct 11, 2021

Choose a reason for hiding this comment

AurumTheEnd Oct 11, 2021

Choose a reason for hiding this comment

AurumTheEnd Oct 11, 2021

Choose a reason for hiding this comment

AurumTheEnd Oct 11, 2021

Choose a reason for hiding this comment

AurumTheEnd Oct 11, 2021

Choose a reason for hiding this comment

judovana commented Oct 14, 2021 • edited by AurumTheEnd Loading

AurumTheEnd commented Oct 14, 2021

judovana commented Oct 15, 2021 via email

AurumTheEnd commented Oct 8, 2021 •

edited

Loading

AurumTheEnd commented Oct 8, 2021 •

edited

Loading

judovana commented Oct 14, 2021 •

edited by AurumTheEnd

Loading