Improve error handling

[larryonoff]

• tasker/______.js adds more error codes, this's a rough implemented feature.
• the most important changes are in tasker/android.js.
• tasker/runServerRemote__.sh container very controversial change. That I think should be removed now or later.

---

This change is 

[czyzm]

Review status: 0 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

tasker/runServerRemote__.sh, line 29 at r1 (raw file):

# so we're skipping errors for 'sudo pkill jx'
# MUST be fixed
! sudo pkill jx

It seems for me as a safety guard so that in case previous jx process is still active we kill it. If there is no jx process running then pkill jx will return exit code 1. In other words returning exit code 1 here is fine. Of course pkill may return 1 also due to other errors and if we would like to be fancy we should check first with pgrep if process exists and just then kill it but I think for now it is enough to leave it as is. Just add comment why we are ignoring the error.

---

Comments from Reviewable

[czyzm]

Reviewed 1 of 3 files at r1. Review status: 1 of 3 files reviewed at latest revision, 2 unresolved discussions.

---

tasker/android.js, line 94 at r1 (raw file):

    if (err ||
        stdout.indexOf(class_name) === -1 ||
        stdout.indexOf('Success') === -1) {

I think we should introduce similar check in runAndroidApp. It also uses adb shell and is just checking exitCode which will be 0 even if app is not started. We could check for existence of 'Error' in the output. Here is the successful run output:

Starting: Intent { cmp=com.rockwellautomation.AppPlatform/.MainActivity }`

and here the failure:

Starting: Intent { cmp=com.rockwellautomation.AppPlatform/.MainActivita }
Error type 3
Error: Activity class {com.rockwellautomation.AppPlatform/com.rockwellautomation.AppPlatform.MainActivita} does not exist.

---

Comments from Reviewable

[czyzm]

Reviewed 1 of 3 files at r1. Review status: 2 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

tasker/__.js, line 28 at r1 (raw file):

  var message = errors[code-1];
  if (message) {
    console.error(errors[code-1], args);

You can use message instead of errors[code-1]

---

Comments from Reviewable

[czyzm]

Review status: 2 of 3 files reviewed at latest revision, 4 unresolved discussions.

---

tasker/__.js, line 80 at r1 (raw file):

    if (out.exitCode != 0) {
      console.log(out.out, "\n");

No need to log it as it will be logged by logErrorAndExit

---

Comments from Reviewable

[czyzm]

Reviewed 1 of 3 files at r1. Review status: all files reviewed at latest revision, 6 unresolved discussions.

---

tasker/__.js, line 83 at r1 (raw file):

      // If jx install fails, we can't run the server
      // and without the server, the tests wouldn't run
      logErrorAndExit(3, [out.exitCode, out.out]);

The code should be 4, not 3 (look at error messages)

---

tasker/__.js, line 106 at r1 (raw file):

      child.on('exit', function (code) {
        logErrorAndExit(4, [code]);

And here should be 5.

---

Comments from Reviewable

[larryonoff]

Review status: 0 of 3 files reviewed at latest revision, 6 unresolved discussions.

---

tasker/__.js, line 28 at r1 (raw file):

Previously, czyzm wrote…

You can use message instead of errors[code-1]

great catch. thank you!

---

tasker/__.js, line 80 at r1 (raw file):

Previously, czyzm wrote…

No need to log it as it will be logged by logErrorAndExit

Done.

---

tasker/__.js, line 83 at r1 (raw file):

Previously, czyzm wrote…

The code should be 4, not 3 (look at error messages)

thanks for catching. I've counted from 0. :(

---

tasker/android.js, line 94 at r1 (raw file):

Previously, czyzm wrote…

I think we should introduce similar check in runAndroidApp. It also uses adb shell and is just checking exitCode which will be 0 even if app is not started. We could check for existence of 'Error' in the output. Here is the successful run output: ``` Starting: Intent { cmp=com.rockwellautomation.AppPlatform/.MainActivity }` ``` and here the failure: ``` Starting: Intent { cmp=com.rockwellautomation.AppPlatform/.MainActivita } Error type 3 Error: Activity class {com.rockwellautomation.AppPlatform/com.rockwellautomation.AppPlatform.MainActivita} does not exist. ```

Done.

---

tasker/runServerRemote__.sh, line 29 at r1 (raw file):

Previously, czyzm wrote…

It seems for me as a safety guard so that in case previous jx process is still active we kill it. If there is no jx process running then pkill jx will return exit code 1. In other words returning exit code 1 here is fine. Of course pkill may return 1 also due to other errors and if we would like to be fancy we should check first with pgrep if process exists and just then kill it but I think for now it is enough to leave it as is. Just add comment why we are ignoring the error.

I added check that verifies that js is running and in this case executes pkill jx

---

Comments from Reviewable

[larryonoff]

Review status: 0 of 3 files reviewed at latest revision, 6 unresolved discussions.

---

tasker/__.js, line 106 at r1 (raw file):

Previously, czyzm wrote…

And here should be 5.

Done.

---

Comments from Reviewable

[czyzm]

Reviewed 2 of 3 files at r2. Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[czyzm]

---

Reviewed 1 of 1 files at r3. Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[jareksl]

tasker/__.js, line 17 at r3 (raw file):

};

var errors = [

NB You could also define errors as object literal, so later you could simply retrieve the message using just code instead of code -1

---

Comments from Reviewable

[jareksl]

---

Comments from Reviewable

[jareksl]

Reviewed 2 of 3 files at r2, 1 of 1 files at r3. Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[larryonoff]

@yaronyg please review too

[yaronyg]

Please fix the index issue but otherwise

---

Reviewed 2 of 3 files at r2, 1 of 1 files at r3. Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

tasker/__.js, line 17 at r3 (raw file):

Previously, jareksl (Jarosław Słota) wrote…

NB You could also define errors as object literal, so later you could simply retrieve the message using just code instead of code -1

+1. Using numeric IDs is really hard to read.

---

Comments from Reviewable

[larryonoff]

Review status: 2 of 3 files reviewed at latest revision, 1 unresolved discussion.

---

tasker/__.js, line 17 at r3 (raw file):

Previously, yaronyg (Yaron Y Goland) wrote…

+1. Using numeric IDs is really hard to read.

Please check if I did it properly.

---

Comments from Reviewable

[jareksl]

tasker/__.js, line 97 at r4 (raw file):

      // If jx install fails, we can't run the server
      // and without the server, the tests wouldn't run
      logAndExit('jxInstallFailed', [out.exitCode, out.out]);

Please note also that you have already object with all codes, so instead of passing string and comparing it in the logAndExit function, easier would be to just pass property like following: logAndExit(errors.indexJsFailed, [out.exitCode, out.out]);

---

Comments from Reviewable

[jareksl]

tasker/__.js, line 70 at r4 (raw file):

  targets.droid = 0;
}
targets.cancel=1;

Since you're editing the code, please add spaces around "=", just to be consistent

---

Comments from Reviewable

[jareksl]

Reviewed 1 of 1 files at r4. Review status: all files reviewed at latest revision, 3 unresolved discussions.

---

Comments from Reviewable

[larryonoff]

Review status: 2 of 3 files reviewed at latest revision, 3 unresolved discussions.

---

tasker/__.js, line 70 at r4 (raw file):

Previously, jareksl (Jarosław Słota) wrote…

Since you're editing the code, please add spaces around "=", just to be consistent

Done.

---

tasker/__.js, line 97 at r4 (raw file):

Previously, jareksl (Jarosław Słota) wrote…

Please note also that you have already object with all codes, so instead of passing string and comparing it in the logAndExit function, easier would be to just pass property like following: logAndExit(errors.indexJsFailed, [out.exitCode, out.out]);

Done.

---

Comments from Reviewable

[jareksl]

Reviewed 1 of 1 files at r5. Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable