Skip to content

Conversation

@bwagner5
Copy link
Contributor

Issue #, if available:
N/A

Description of changes:

  • Add e2e tests for primitive filters

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2020

Codecov Report

Merging #41 into master will increase coverage by 0.18%.
The diff coverage is n/a.

Impacted file tree graph

@@            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     
Impacted Files Coverage Δ
pkg/selector/selector.go 82.83% <0.00%> (+0.85%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a67979b...64a6325. Read the comment docs.

@bwagner5 bwagner5 requested a review from haugenj July 29, 2020 19:18
AEIS="${BUILD_DIR}/ec2-instance-selector"

## Build binary for tests to run
make -f $MAKEFILE build
Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@brycahta brycahta left a comment

Choose a reason for hiding this comment

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

Looks good👍🏼

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