-
Notifications
You must be signed in to change notification settings - Fork 162
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
Add credo to CI tests #316
Conversation
does ebert not provide the same functionality as credo? |
scripts/ci_tests.sh
Outdated
mix "$TEST_COMMAND" $INCLUDED_TESTS | ||
fi | ||
# Retry if it doesn't work the first time | ||
mix "$TEST_COMMAND" $INCLUDED_TESTS || mix "$TEST_COMMAND" $INCLUDED_TESTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double quote to prevent globbing and word splitting.
scripts/ci_tests.sh
Outdated
mix "$TEST_COMMAND" $INCLUDED_TESTS | ||
fi | ||
# Retry if it doesn't work the first time | ||
mix "$TEST_COMMAND" $INCLUDED_TESTS || mix "$TEST_COMMAND" $INCLUDED_TESTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double quote to prevent globbing and word splitting.
It does, but this gives the extra layer of ensuring that credo passes, or it will fail the test suite. It's currently passing, which was our roadblock in the past to enabling it, if I recall correctly. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Josh just wants a t-shirt.
🐐 as-is but a couple comments
Do we want to add a .credo.exs?
scripts/ci_tests.sh
Outdated
@@ -6,8 +6,15 @@ | |||
# | |||
# This script could be used for local testing as long as COVERALLS is not set. | |||
|
|||
set -e |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of this, do we want to run both and then have it report which things failed?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set -ex
probably will be more obvious on which failed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
mix "$TEST_COMMAND" $INCLUDED_TESTS | ||
fi | ||
# Retry if it doesn't work the first time | ||
mix "$TEST_COMMAND" $INCLUDED_TESTS || mix "$TEST_COMMAND" $INCLUDED_TESTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double quote to prevent globbing and word splitting.
mix "$TEST_COMMAND" $INCLUDED_TESTS | ||
fi | ||
# Retry if it doesn't work the first time | ||
mix "$TEST_COMMAND" $INCLUDED_TESTS || mix "$TEST_COMMAND" $INCLUDED_TESTS |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Double quote to prevent globbing and word splitting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is incorrect. Quoting this changes the meaning of the arguments.
Ebert has finished reviewing this Pull Request and has found:
You can see more details about this review at https://ebertapp.io/github/kafkaex/kafka_ex/pulls/316. |
Fixes #100