-
Notifications
You must be signed in to change notification settings - Fork 102
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
e2e tests #41
e2e tests #41
Conversation
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
==========================================
+ Coverage 88.38% 88.57% +0.18%
==========================================
Files 9 9
Lines 1068 1068
==========================================
+ Hits 944 946 +2
+ Misses 88 87 -1
+ Partials 36 35 -1
Continue to review full report at Codecov.
|
test/e2e/run-test
Outdated
AEIS="${BUILD_DIR}/ec2-instance-selector" | ||
|
||
## Build binary for tests to run | ||
make -f $MAKEFILE build |
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.
Why build here instead of having build be part of the make e2e-test
target?
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.
that's a good point. I wanted it to be standalone executable, but I suppose if the binary doesn't exist, it would just error. If it was out-of-date though, could get bad results.
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.
any particular reason why you want it to be standalone executable?
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.
I think the only reason would be if they grow and we'd need to add some CLI options to fail fast or do some other optimization when running them. Was interested in using local-stack (they don't currently support DescribeInstanceTypes or DescribeInstanceTypeOfferings, but maybe in the future) so that they could be run without credentials, which could be a CLI option. Make doesn't make it easy to pass options like that. But I think simplifying it now to only be run through make is fine. If we end up adding stuff later, we can reevaluate, or do a more clever check on the binary to make sure the latest version is being executed.
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.
Looks good👍🏼
Issue #, if available:
N/A
Description of changes:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.