Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Return code successful on task failure #35

Closed
mbrodala opened this issue Aug 27, 2019 · 7 comments
Closed

Return code successful on task failure #35

mbrodala opened this issue Aug 27, 2019 · 7 comments
Labels

Comments

@mbrodala
Copy link
Collaborator

mbrodala commented Aug 27, 2019

Given the following Runnerfile.sh:

task_test() {
  return 1
}

When running this task I see this output:

$ runner test
[01:25:19.119] Starting 'test'...
[01:25:19.127] Task 'test' failed after 2 ms (1)
$ echo $?
0

The overall return code indicates success, any idea what's wrong here? Could this be related to #32?

@mbrodala
Copy link
Collaborator Author

OK, apparently this goes deeper. If I drop the task again and retry I get this:

$ runner test
[01:28:41.317] Task 'test' is not defined!
$ echo $?
0

But again, the status code indicates success ...

@mbrodala
Copy link
Collaborator Author

And one more finding: the return code apparently works fine for the implicit default task.

Given this Runnerfile.sh:

task_default() {
  false
}

The behavior is as expected:

$ runner
[01:34:28.467] Starting 'default'...
[01:34:28.479] Task 'default' failed after 2 ms (1)
$ echo $?
1

However, explicitly invoking that task shows the wrong behavior again:

$ runner default
[01:35:13.271] Starting 'default'...
[01:35:13.279] Task 'default' failed after 1 ms (1)
$ echo $?
0

@mbrodala
Copy link
Collaborator Author

After a little debugging I found that runner_sequence() swallows the actual task return code. A small fix could be this:

-    runner_run_task "${task}" || return
+    runner_run_task "${task}" || return $?

Could it be that a plain || return is a bad idea?

@stylemistake
Copy link
Owner

I had that issue with plain returns, and i thought most of them were fixed. It is supposed to implicitly return the last exit code, but with such bugs popping up I am no longer sure.

Good catch.

@mbrodala
Copy link
Collaborator Author

@stylemistake Any updates on this? We had to gradually drop the Task Runner in all of our builds due to this ...

@stylemistake
Copy link
Owner

stylemistake commented Apr 29, 2020

@mbrodala I have pushed a fix, along with a test for your case. Check if it works for you, and if you need a new release on npm, let me know.

P.S. You could've submitted a pull request for that kind of thing. I'm a pretty chill guy at accepting PRs. :)

@mbrodala
Copy link
Collaborator Author

I wasn't 100% sure that my fix was the fix but evidently it works now, thanks. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants