ceylon test command should exit with nonzero status if tests fail

[lucaswerkmeister]

Currently, ceylon test X always exits with status 0. For automation purposes, it would be much more convenient if it exited with 0 only in the case of success, and some other status otherwise (perhaps with different statuses for “tests failed” or “abnormal failure” as well).

Not sure whether this belongs in the Ceylon tool or the Java tool. Probably the latter, but the former will have to inform the latter of the outcome somehow.

[thradec]

Done.

[lucaswerkmeister]

Thanks!

[tombentley]

The fix for this has broken the embedded use case because now the tool causes the process to exit. In particular we used to run ceylon test in the compiler tests (specifically to test ceylon/ceylon-compiler#1572) and this now exits the JVM in which the tests are running.

[lucaswerkmeister]

So the Runner needs to tell the CeylonRunTool the exit code even though the latter only uses CeylonRunTool. [System.setProperty("com.redhat.ceylon.testjvm.exitCode", exitCode)](https://docs.oracle.com/javase/7/docs/api/java/lang/System.html#setProperty%28java.lang.String, java.lang.String%29)?

[luolong]

This does not sound right.

Can't the run tool just return with the Success/Failure status, that can be read and interpreted by the embedded runner?

[tombentley]

Well what's the right API to use to do ceylon test ... programmatically? Because whatever that is, it shouldn't be exit()ing, but leaving it to its caller to exit or do whatever else.

[thradec]

Well what's the right API to use to do ceylon test ... programmatically? Because whatever that is, it shouldn't be exit()ing, but leaving it to its caller to exit or do whatever else.

The api is createTestRunner(module).run(), and it doesn't call exit().

[tombentley]

Well maybe that's the one we should be using in the compiler tests then.

But is there a good reason why we can't use the CeylonTestTool directly? I mean the intention with the tool API was the tools were just Java Beans and if a tool needs to exit abnormally it should just throw and the framework should catch that and end up try to exit with a non-zero status code.

[thradec]

But is there a good reason why we can't use the CeylonTestTool directly? I mean the intention with the tool API was the tools were just Java Beans and if a tool needs to exit abnormally it should just throw and the framework should catch that and end up try to exit with a non-zero status code.

I don't see any way how to exit with custom code, without calling exit() from com.redhat.ceylon.testjvm/js. In jvm it would be possible to throw exception and handle it in tool, but it doesn't work for js.

[thradec]

btw. if it is just for one test in compiler, it would be possible to override SecurityManager.checkExit

[gavinking]

Folks WTF is going on here? This trivial issue has been open for >6 months now. Can I close it please?

[FroMage]

So here's what I think. I'll add a --no-exit to the JVM Ceylon implementation of the test tool, so it will throw instead of exiting. It's useless for the JS impl since we can't catch exceptions from node.js when calling it in the runjs tool.

By default the CeylonTestTool and CeylonTestJsTool will still exit with error code (that is what we want from the command-line). But I will add an option to not make it so that the CeylonTestAntTask and CeylonTestJsAntTask will not exit, because that's very hard to believe this is what we want, right?

[FroMage]

BTW, apparently ATM the CeylonRunJsAntTask will call exit on error. I also think that's wrong, no?

[FroMage]

Right, I just checked and it does. It also does on the JVM but since we fork for the CeylonRunAntTask we don't notice. So I believe that on the JVM calling exit by default is right, unless we don't fork, in which case we should throw. What a mess. I'll fix all that.

So also, the CeylonRunJsTool calls exit by default on error (though there's a flag to turn it off). The CeylonRunTool does not, and will throw, which is caught by CeylonTool which passes the info back to Launcher who does the exit. I think the JS tool should do the same. I'll fix that too.

[FroMage]

So to recap what I will do:

For JS:

• the ceylon test tool will always call exit (the only way to communicate with the tool)
• CeylonRunJsTool will never call exit (it's Launcher's job), but will throw if node.js returns an exit code (with that exit code in the exception)
• CeylonTool catches that exit code and passes it to Launcher
• Launcher sees non-zero exit codes and exits with them, when invoked from the CLI, else just returns the code to the caller
• CeylonTestJsTool delegates to CeylonRunJsTool so it will throw as well
• CeylonRunJsAntTask and CeylonTestJsAntTask use Launcher to get the exit code and properly log a build failure

For JVM:

• the ceylon test tool will never call exit: it will throw an AssertionError on failure
• CeylonRunTool still never call exit (it's Launcher's job), but will let any exception through
• CeylonTool catches that exception and passes an exit code to Launcher
• Launcher sees non-zero exit codes and exits with them, when invoked from the CLI, else just returns the code to the caller
• CeylonTestTool delegates to CeylonRunTool so it will throw as well
• CeylonRunAntTask and CeylonTestAntTask use Launcher in a new JVM, which will throw and return an exit code, which we catch and properly log a build failure

So it should all work out.

[FroMage]

OK all done.

[thradec]

@FroMage after this change, the console output contains exception stacktrace caused by assert(result.isSuccess), which is not very useful for user.

======================= TESTS FAILED ! =======================
Exception in thread "main" ceylon.language.AssertionError "Assertion failed: Tests failed
    violated result.isSuccess"
    at com.redhat.ceylon.testjvm.Runner.run(tool.ceylon:86)
    at com.redhat.ceylon.testjvm.run_.run(tool.ceylon:36)
    at com.redhat.ceylon.testjvm.run_.main(tool.ceylon)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:606)
    at ceylon.modules.api.runtime.SecurityActions.invokeRunInternal(SecurityActions.java:57)
    at ceylon.modules.api.runtime.SecurityActions.invokeRun(SecurityActions.java:48)
    at ceylon.modules.api.runtime.AbstractRuntime.invokeRun(AbstractRuntime.java:72)
    at ceylon.modules.api.runtime.AbstractRuntime.execute(AbstractRuntime.java:119)
    at ceylon.modules.api.runtime.AbstractRuntime.execute(AbstractRuntime.java:103)
    at ceylon.modules.Main.execute(Main.java:69)
    at ceylon.modules.Main.main(Main.java:42)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:606)
    at org.jboss.modules.Module.run(Module.java:312)
    at org.jboss.modules.Main.main(Main.java:460)
    at ceylon.modules.bootstrap.CeylonRunTool.run(CeylonRunTool.java:244)
    at ceylon.modules.bootstrap.CeylonTestTool.run(CeylonTestTool.java:172)
    at com.redhat.ceylon.common.tools.CeylonTool.run(CeylonTool.java:491)
    at com.redhat.ceylon.common.tools.CeylonTool.execute(CeylonTool.java:380)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:606)
    at com.redhat.ceylon.launcher.Launcher.runInJava7Checked(Launcher.java:114)
    at com.redhat.ceylon.launcher.Launcher.run(Launcher.java:41)
    at com.redhat.ceylon.launcher.Launcher.run(Launcher.java:34)
    at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
    at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:57)
    at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
    at java.lang.reflect.Method.invoke(Method.java:606)
    at com.redhat.ceylon.launcher.Bootstrap.run(Bootstrap.java:55)
    at com.redhat.ceylon.launcher.Bootstrap.main(Bootstrap.java:32)

[thradec]

btw. with this approach, it is not possible to exit with custom code, to distinguish what fails

[FroMage]

Yes, for both backends. I could catch it in the ant task, I guess. I figured it was OK as long as we log a failure. But I could hide it, yes.

btw. with this approach, it is not possible to exit with custom code, to distinguish what fails

Why not? It still calls exit with the right exit code, no? Oh, you mean the assert exception. Well OK I can make an exception that contains the exit code I suppose, but how many exit codes do you expect to have to handle? Tests either pass or they don't, no?

[thradec]

... but how many exit codes do you expect to have to handle? Tests either pass or they don't, no?

hypothetically (but it is not something what I would like to implement now) there can be following results: success, test failed (propagate assertion exception), test with error (propagate non-assertion exception), test framework crashed

but for now it will be enough to not show the stacktrace ;-)

[FroMage]

Should be fixed now.