Mismatch between treatment of invalid patches between Internal and External test runner

[justynapt]

Basically patchValid check is not performed in ExternalTestRunner. Spoted by @GiovaniGuizzo , thank you. We need to decide on default behaviour and consistently apply it for both.

[sandybrownlee]

On which line numbers is there an inconsistency? I must be missing something: it looks the same in both Internal and External TR to me. (the if (patchValid) condition is commented out on both)

[justynapt]

https://github.com/gintool/gin/blob/master/src/main/java/gin/test/InternalTestRunner.java#L73-L75 https://github.com/gintool/gin/blob/master/src/main/java/gin/test/ExternalTestRunner.java#L124-L128 (more details sent via email)

[sandybrownlee]

Got it. Definitely a bug - I think the desired behaviour should be to include the check (as InternalTestRunner does)

[GiovaniGuizzo]

I think the ExternalTestRunner code is a bit better, as it does not run the tests when the patch is invalid. InternalTestRunner would still run the tests for the invalid patch.

A possible solution (and what I coded in my working branch) is:

if (compiledOK && patchValid) {
     results = runTests(reps);
} else {
    results = emptyResults(reps);
}

Maybe we can standardise in the InternalTestRunner class as well.

[sandybrownlee]

I'm possibly confused between the two! The code you suggest is what I had in mind.

[GiovaniGuizzo]

I'm possibly confused between the two! The code you suggest is what I had in mind.

Oh, good. I was referring to this piece of code from InternalTestRunner:

if (compiledOK) {
    classLoader.setCustomCompiledCode(this.getClassName(), code.getByteCode());
    results = runTests(reps, classLoader);
}

if (!patchValid || !compiledOK) {
    results = emptyResults(reps);
}

The first check misses "patchValid", so it unnecessarily runs the tests on invalid patches. The code from ExternalTestRunner is better in the sense that it avoids running irrelevant patches.

[sandybrownlee]

Your suggest code is neater than both!

[GiovaniGuizzo]

I just added the patchValid check to the originally neat code of ExternalTestRunner :)