apache / cordova-paramedic

Apache Cordova - Paramedic
https://cordova.apache.org/
Apache License 2.0
36 stars 53 forks source link

Builds with Appium tests pass even if Jasmine tests fail #179

Open janpio opened 5 years ago

janpio commented 5 years ago

During development of https://github.com/apache/cordova-paramedic/pull/178 I noticed, that the tests were passing although there were Jasmine (main) test failures:

https://travis-ci.org/apache/cordova-paramedic/jobs/552245365

Seems it is enough if the Appium tests pass for the whole run to be counted as successful. Not good.

janpio commented 5 years ago

https://github.com/apache/cordova-paramedic/pull/178 is blocked by this, because it makes the failing tests invisible for runs with Appium.

janpio commented 5 years ago

Does anyone know where exactly the success/failure of a run is defined?

Because I added some output of what I though it was, but that indicates I didn't understand correctly: https://travis-ci.org/apache/cordova-paramedic/jobs/552280079

cordova-paramedic results (SauceLabs): isTestPassed = false, isAppiumTestPassed = true

https://github.com/apache/cordova-paramedic/pull/178/files#diff-ff902472818c9b031ca73ec778f68526 I thought that last return would bubble up to Travis, but as this would be false here that can't be the case.

janpio commented 5 years ago

ping @alsorokin

alsorokin commented 5 years ago

isTestPassed used to be an indicator of successful test run. Maybe this .catch or .timeout gets in the way? https://github.com/apache/cordova-paramedic/blob/master/lib/paramedic.js#L95-L100

janpio commented 5 years ago

I don't think so - that all looks perfectly fine. The problem is the other way around: one of the two TestPassed vars are false, but paramedic still reports success to Travis.

alsorokin commented 5 years ago

Ah, I see. But also I think it wasn't false but undefined. So there are two problems here, it seems:

janpio commented 5 years ago

Why do you think undefined?

I added some output, see the earlier comment: https://github.com/apache/cordova-paramedic/issues/179#issuecomment-506995062

whatever the result was, it doesn't get accounted

Yep, that I agree with. I don't understand the code where this gets returned actually. I just can't find out what is executed as the last thing and defines that exit status.

alsorokin commented 5 years ago

Because of the line Completed tests at 23:26:58 with result: undefined

janpio commented 5 years ago

Oh, I didn't notice that. Looking into it.

janpio commented 5 years ago

Strange, this doesn't seem to influence the result: https://travis-ci.org/apache/cordova-paramedic/jobs/552280069 vs. https://travis-ci.org/apache/cordova-paramedic/jobs/552280077

failing build:

cordova-paramedic results (SauceLabs): isTestPassed = false, isAppiumTestPassed = true
Completed tests at 23:21:35 with result: undefined

(wrongly) succeeding build:

cordova-paramedic results (SauceLabs): isTestPassed = false, isAppiumTestPassed = true
Completed tests at 23:26:49 with result: undefined
janpio commented 5 years ago

I notice that the wrongly succeeding ones mess up on "Getting saucelabs jobs details..." - there is nothing output for some reason. That generally happens when the Appium tests are actually run.

alsorokin commented 5 years ago

It seems paramedic could use some more refactoring ;)

janpio commented 5 years ago

I don't disagree - more or less what we are doing right now. Problem is understanding what is actually going on in the current code.

Like this:

cordova-paramedic results (local): isTestPassed = true, isAppiumTestPassed = true: result = true
---------------------------------------------------------
6. Collect data and clean up
---------------------------------------------------------
Completed tests at 8:48:44 AM with result: undefined

So that finis definitely not getting the return value (https://github.com/apache/cordova-paramedic/blob/084dd2b3b6d9bbbb31bb7b6c4d7a100331d6bfec/lib/paramedic.js#L101-L104)


But: Then this also can't be where the actual return status code of the whole paramedic run is set anyway.