-
Notifications
You must be signed in to change notification settings - Fork 508
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 tests for graphQL costs #1643
Conversation
Integration tests success for |
Codecov Report
@@ Coverage Diff @@
## main #1643 +/- ##
==========================================
+ Coverage 56.12% 58.97% +2.84%
==========================================
Files 72 72
Lines 6493 6493
==========================================
+ Hits 3644 3829 +185
+ Misses 2604 2413 -191
- Partials 245 251 +6 |
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.
Awesome! Thanks
efdbb40
to
d91ca5a
Compare
Integration tests success for |
d91ca5a
to
981f99d
Compare
Integration tests success for |
981f99d
to
387e188
Compare
Integration tests success for |
@@ -261,7 +261,7 @@ test: $(test-targets) | |||
unit-test: ## Runs unit test without e2e | |||
# Run unit tests, ignoring e2e tests | |||
# run the go tests and gen the file coverage-all used to do the integration with codecov | |||
go test -race -covermode=atomic -coverprofile=unit-coverage.out `go list ./... | grep -v e2e` | |||
SKIP_GINKGO=1 go test -race -covermode=atomic -coverprofile=unit-coverage.out `go list ./...` |
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.
Is there a reason these e2e tests couldn't sit in the e2e dir?
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.
@06kellyjac Nice suggestion, Thanks. Do you want to do a PR for the change?
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.
The tests added in this PR rely on some internal (non-exposed) fields. So, had to create the tests in the same package as it was testing.
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.
fair enough, thanks for the details
What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Closes BUG: Fixer high GitHub API usage #1623
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
No.