-
Notifications
You must be signed in to change notification settings - Fork 471
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 blackbox tests for serve command #608
Add blackbox tests for serve command #608
Conversation
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, just needs to be hooked into CI
@@ -36,6 +36,14 @@ bench: | |||
$(info INFO: Starting build $@) | |||
go test -bench=. | |||
|
|||
alpha-test-%: release/goss-% |
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.
When does this get 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.
via ci/build.sh
in the if darwin or windows bit.
(Honestly, Makefiles are not a strong suit for me, so I haven't wired there)
Makefile
Outdated
$(info INFO: Starting build $@) | ||
./integration-tests/run-tests-alpha.sh $* | ||
|
||
test-serve-%: release/goss-% |
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.
make test should execute this as part of it's testing, so that it's part of CI.
I was able to test it locally with make test-serve-linux-amd64
and it looks great.
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.
It's wired via ci/build.sh
- or have I made a mistake?
9983e32
to
fadc66a
Compare
@aelsabbahy I think that it already is via |
I'm fine with build.sh calling one make task, but I don't want build.sh to turn into an orchestration layer of multiple tasks or make calls. Basically, for local development a user should be able to run |
I've had a go, please take a look? It's complicated by needing to filter to the current OS that is being executed on, so it sounds like |
I'll take a look over the weekend. Thanks for making the changes. If you don't mind can you send me an order list of all the PRs that are ready to go and which order you prefer them merged in (if any). I haven't used bazel (outside or demoing it), but I love the concepts behind it and nixos for hermetic builds. |
integration-tests/run-serve-tests.sh
Outdated
if curl --silent "http://127.0.0.1:${open_port}/healthz" | grep 'Count: 2, Failed: 0, Skipped: 0' ; then | ||
echo "passed" | ||
else | ||
echo "failed, exit code $?" |
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 this supposed to fail? It just echo's out a message and succeeds.
I added a:
ret=$?
exit $ret
but that didn't work because of the trap
maybe:
ret=$?
and a exit "$ret"
at the bottom of cleanup?
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.
Yeah, that was supposed to fail. Fixed inside 23ce6f1. Good catch - sorry!
27f7049
to
23ce6f1
Compare
@aelsabbahy I've rebased and also fixed the lack of failure-on-tests-failing. |
Ci test failure appears related to this change. I think this PR is really close.. but maybe some intermittent issues? |
90770b5
to
dffb864
Compare
I've rebased and made no changes that I think could have fixed some problem, but it is green now. |
Restarted the build 3 times and it was green all three times, so it might be intermittent. |
Checklist
make test-all
(UNIX) passes. CI will also test thisDescription of change
Added a cross-platform blackbox test for
goss serve
to give some outside in safety to #606 and incoming output-format-via-http-content-negotiation change.Also integrated the alpha testing into the
Makefile
for coherence with the others. I'm not a comfortable make-r, so please critique brutally if I've done things poorly?