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

Test coverage uses bash #24

Merged
merged 2 commits into from
Feb 3, 2016
Merged

Test coverage uses bash #24

merged 2 commits into from
Feb 3, 2016

Conversation

seppo0010
Copy link
Contributor

pushd/popd are not available in sh

Oops, my bad here. I should have tested it. I cannot wait for Travis integration.

pushd/popd are not available in sh
@@ -1,4 +1,4 @@
#!/usr/bin/env sh
#!/usr/bin/env bash

set -e
rm -rf lcov _build _bin && mkdir _build && pushd _build && cmake -DCOVERAGE=on .. && make -j && make test && popd

This comment was marked as spam.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is pushd/popd every necessary?

I think it is desirable that if something fails (like a test) it will leave the user in the same location they were.

This comment was marked as spam.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bash makes sure that popd is called automatically if the script fails. set -e (and &&) bails when any command fails and prevents further commands from being executed. For example:

~/Desktop $ cat ./example.bash
#!/usr/bin/env bash

set -e
mkdir -p test
pushd test
rm -rf fake-directory
ls fake-directory
echo "not called"
popd test
~/Desktop $ ./example.bash
~/Desktop/test ~/Desktop
ls: fake-directory: No such file or directory
~/Desktop $ echo $?
1

Implementing this without pushd/popd and using pwd instead is possible, but it would require an overhead of checking the return status of each command 😨.

This comment was marked as spam.

@seppo0010
Copy link
Contributor Author

ping @thinkingfish

@kevyang
Copy link
Contributor

kevyang commented Dec 8, 2015

lgtm, ship it (given you've tested in osx and gnu/linux)

thinkingfish pushed a commit that referenced this pull request Feb 3, 2016
@thinkingfish thinkingfish merged commit 3c44d3e into master Feb 3, 2016
@thinkingfish thinkingfish deleted the test-coverage-bash branch February 6, 2016 01:08
michalbiesek added a commit to michalbiesek/pelikan that referenced this pull request Jun 28, 2019
swlynch99 pushed a commit to swlynch99/pelikan-twitter that referenced this pull request Sep 30, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants