testdouble / scripty

Because no one should be shell-scripting inside a JSON file.
MIT License
963 stars 23 forks source link

Exit-code handling #38

Closed greduan closed 8 years ago

greduan commented 8 years ago

ATM I have some unit tests structured as such:

    "test": "scripty",
    "test:clean": "scripty",
    "test:knexreset": "scripty",
    "test:unit": "scripty",
    "test:integration": "scripty",
    "test:integration:installation": "scripty",
    "test:integration:manager": "scripty",

The contents of the files:

## /scripts/test/index
#!/usr/bin/env sh

npm run test:unit
npm run test:integration

## /scripts/test/unit
#!/usr/bin/env sh

# run some test commands

## /scripts/test/integration/installation
#!/usr/bin/env sh

# run some test commands

## /scripts/test/integration/manager
#!/usr/bin/env sh

# run some test commands

The problem I'm having is that when I run npm test, when one of those subscripts fails it doesn't exit with a non-zero code, so the continuous integration thinks that the tests passed.

What is a way to fix this?

For the moment I'm just running npm run test:unit && npm run test:integration:manager && npm run test:integration manually, so that each one does produce a proper error code on exit and the tests fail if one of them fails.

searls commented 8 years ago

Woah, if that's true, that is definitely a bug.

Could you post a project that reproduces the problem? Or to the repo's example project? Or add a failing test?

greduan commented 8 years ago

Yeah, I'll try to get something going today as a proof of concept.

greduan commented 8 years ago

Got a repo ready for you, along with lengthy explanation of my observations in the README.md file: https://gitlab.com/Greduan/scripty-erroneous-error-handling-pof

searls commented 8 years ago

Thanks.

This is an area I stupidly punted on because I didn't know how to reduce multiple exit codes to a single one.

Maybe I should just add them all together.

greduan commented 8 years ago

Not sure what the internals are so I can't really suggest any real solutions, but my use case is simply for it to exit with an error code that isn't 0 whenever one of the commands fails, that's all.

mike-engel commented 8 years ago

I'm having the same problems, and for my team's use cases it's enough to return immediately on the first non-zero exit code. So if you have two commands being run in the same scripty script, if the first one returns non-zero, it's fine to exit there with that status.

greduan commented 8 years ago

@mike-engel I believe one workaround is the following:

npm run yourcommand || exit 1 # $? if you want to preserve the command's exit status

|| is the opposite of &&, it triggers if the exit code is not 0, then you just make the script exit.

searls commented 8 years ago

Root cause: the fact that scripts/test contains invocations of npm run … was probably a red herring, because it seemed like the sort of thing that scripty should know about and handle. In truth, because scripts/test was a custom script, scritpy doesn't have any visibility into what's inside of it. So the fact is that because the shell script above will exit 0 because of echo, it will also exit 0 when spawned by scripty.

By adding the set -e flag to the script, it'll fail fast and the script will exit non-zero, scripty will see it, and will also fail non-zero.

If you wanted to get this functionality out of the box, you could have scoped one and two underneath a directory named scripts/test and then, when running npm test, scripty would run both test:one and test:two and exit non-zero if either failed (with the exit code of the last failure)

searls commented 8 years ago

I submitted a PR to demonstrate: https://gitlab.com/Greduan/scripty-erroneous-error-handling-pof/merge_requests/1

greduan commented 8 years ago

@searls is that for @mike-engel or me? Or both?

In any case, set -e would work for fail-fast.

Though what if I don't want it fail-fast? What if I want to run all the tests just to see if everything else passes and not have to run the CI again just to see that.

The reason I'm doing npm run one and npm run two inside of scripts/test/index is because I do not want to run all the scripts that are under scripts/test/, in my real example it would be that I don't want to run test:clean and test:knexreset, as those are used inside the other ones that do need them.

Though I guess I could just do clean:clean and clean:knexreset or something like that.

searls commented 8 years ago

The comment was directed at the broader issue, I suppose.

If you don't want to fail fast, then you'd need to find a scripting approach to accomplish it within the script. Since sh is going to return 0, there's nothing scripty can do to enforce a failure.

greduan commented 8 years ago

Thought it'd be something like this, I also couldn't come up with a way to keep track of the escape codes of the innards of a shell script. I'll have to be cleverer about the script structure.

Thanks for your time. :)

jasonkarns commented 8 years ago

If your script returns 0, it was a success. If it returns nonzero, it failed.

If you don't want your script to fail fast, you can capture each of the return codes from the commands within your script and coalesce/convert as desired. The important thing to remember is that scripty (as is true of shell programming in general) has no knowledge of the commands that occur within the script. The contract that shell programs rely on is:

Scripty can only be aware of the exit code of the command it runs. So if your script returns 0, scripty sees it as a success; return nonzero: failure. set -e is commonly (no judgement here) an easy way out of dealing with exit codes. But all that does is ensures the script returns with the same code as the first non-zero command.