Issue #119: Guarantee executing popOperand() in popUninitializedVariableOperand() via moving popOperand() out of "assert"

[HeartSaVioR]

There's a bug discovered in #119 which seems to execute "stateful" operation into "assert". The execution is not guaranteed, and leads to inconsistent stack map.

This patch adds new UT to reproduce #119 - it fails on latest master branch, and passes with this PR.

[HeartSaVioR]

It would be ideal if we run the entire tests twice via assert option on and off; not sure Janino build does that.

[HeartSaVioR]

#121 to enable Travis CI build which helps to figure out such case.

[aunkrig]

Merged this PR, but (intentionally!) not the unit test, because that unit test may have side effects (setting / resetting assertion status).

[aunkrig]

It would be ideal if we run the entire tests twice via assert option on and off; not sure Janino build does that.

Strictly speaking, you're right. However this bug is a singleton, and the very small risk that "it will occur again" does not justify the effort/time to run the regression tests twice.

Alas, what is much more critical, is to run all the tests agains several JREs (at least 6, 7, 8 and 11), which is extremely difficult because JUNIT doesn't support that. I once wrote a JUNIT TestClassRunner that attempts to implement that

https://svn.code.sf.net/p/loggifier/code/de.unkrig.commons/trunk/commons-junit4/src/main/java/de/unkrig/commons/junit4/runner/MultipleJresTestClassRunner.java

, but it is terribly slow and clumsy.

So the unit tests that are executed by maven are only regarded as "sanity testing", while I do the really careful regression testing manually with ECLIPSE.

Maybe I should open a to-do issue for extending the UTs with the MultipleJresTestClassRunner.

[HeartSaVioR]

Thanks for reviewing and merging!

Merged this PR, but (intentionally!) not the unit test, because that unit test may have side effects (setting / resetting assertion status).

That's OK, as it turned out other unit tests were failing based on the assertion status.

However this bug is a singleton, and the very small risk that "it will occur again" does not justify the effort/time to run the regression tests twice.

While the bug was a singleton, we had no idea what is happening, even there're a couple of 3.1.x versions released already. That's the matter of "stability" - given Janino is more likely used as a library, the users would expect more stability; showing the efforts of striving to guarantee stability has been already the first class priority in most of open source projects, and that's why other projects have been adopted various integration tools like CI, test coverage, etc.

Alas, what is much more critical, is to run all the tests agains several JREs (at least 6, 7, 8 and 11), which is extremely difficult because JUNIT doesn't support that.
... , but it is terribly slow and clumsy.

...and which is very trivial to add in Travis CI build with running in parallel.

https://travis-ci.org/github/HeartSaVioR/janino/builds/663233350

(Not fully parallel though - looks like 5 parallel builds allowed per a trigger, so two groups of builds happen. If you reduce the JDK to 2 flavors, they would be grouped with one group, but can't we wait for just 10 mins?)

(OpenJDK 11 seems to fail due to Javadoc issue which I'm not aware of - that can be fixed or applied workaround once we have a plan to adopt it.)

OpenJDK 10 build failure was from:

test_15_27_1__Lambda_parameters[CompilerFactory=jdk](org.codehaus.commons.compiler.tests.JlsTest)  Time elapsed: 0.02 sec  <<< ERROR!
org.codehaus.commons.compiler.CompileException: Line 1, Column 51: 'var' is not allowed here (compiler.err.var.not.allowed.here)
	at org.codehaus.commons.compiler.tests.JlsTest.test_15_27_1__Lambda_parameters(JlsTest.java:2753)

I once wrote a JUNIT TestClassRunner that attempts to implement that
https://svn.code.sf.net/p/loggifier/code/de.unkrig.commons/trunk/commons-junit4/src/main/java/de/unkrig/commons/junit4/runner/MultipleJresTestClassRunner.java

Unless the class can "detect" the path of JREs, this change would make others (including me) stop to contribute Janino or stuck to 3.0.x - I'm MacOS/IntelliJ user, and most of Spark contributors are also Linux/MacOS users. It even seems to be sensitive to bugfix version as well - if you upgrade JRE you'll be going to fix the code. Why not let the existing great tools do it for you instead?

[aunkrig]

Oh, MultipleJresTestClassRunner is in its infancy, and the hard-coded JAVA_HOME paths are certainly not to last.

However I fear that I don’t get the MJTCR running... the JUNIT APIs are more complex than you'd expect. So I'll definitely look into TRAVIS.