-
Notifications
You must be signed in to change notification settings - Fork 107
Conversation
@replay what do you think of this? |
8aa2476
to
518ad1f
Compare
seems like in practice, the old code is faster on circleCI, even though on my computer new is about twice as fast 🎱 to be clear, the speedup is supposed to come from the new approach being more eager (e.g. it'll retry the request until it gets a valid one, whereas the old approach just waits 30 seconds before checking) edit: seems like the slowdown is coming from the teardown of the stack at the end |
Seems ok. But what is actually the point of replacing the end2end test? isn't that just doing the same? |
there isn't a significant amount of point. that's why this also isn't high prio or anything, but I do think it's better that towards the future that all our different verification tooling converges on a set of standardized helper utilities for validating responses. |
will be useful for carbon tests
otherwise got errors like 0: already closed
1) it's about twice as fast (on my system at least) ./scripts/qa/end2end.sh 1.17s user 0.18s system 2% cpu 1:05.40 total go test -v ./tests/simple_carbon 1.35s user 0.20s system 5% cpu 30.795 total 2) reuse of shared helper libraries with chaos stack. these helper libraries are good for all kinds of testing, not just chaos-induced 3) we're better at Go than at python
with previous approach, while docker-compose up was running, bash would just wait on it, and be unable to invoke its trap handler. apparently, when running the script interactively, ctrl-C would reach docker-compose up directly, then invoke the trap handler, but when running the script as subprocess, the signal wouldn't reach anyone. see https://unix.stackexchange.com/questions/146756/forward-sigterm-to-child-in-bash
so we don't get connection/write errors when cleaning up the tests and shutting down the docker stack.
* context for Command -> when you call cancelFunc() it uses sigkill we need the process to be able to clean up elegantly, so we need a more gentle signal this was causing us not to be able to shut down the tests properly * no need for context in tracker either, because we can simply complete reads before waiting for command
so you can see timing info everywhere needed
516c3fb
to
522420b
Compare
update: makes sense to merge, especially now that the fakemetrics tool has seen a bunch of changes; with the old tooling, would need to somehow pin the fakemetrics binary to the exact right version, but with this code, it's trivial to assert that the tooling is in sync. |
should have been part of #830
should have been part of #830
should have been part of #830
stacktest/tests/end2end_carbon/end2end_carbon_test.go
file