-
Notifications
You must be signed in to change notification settings - Fork 507
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
test: add integration tests for upgrades #2260
Conversation
5dd20a8
to
8d44507
Compare
f9d837d
to
d9d1fdf
Compare
Seems not too fail locally! I think this'll make our CI quite a bit slower, but I'll like to ignore that for now, and revisit some time later hollistically, with the goal of making the overall CI faster. |
1b8ec6b
to
dcc0402
Compare
.github/workflows/ci.yml
Outdated
@@ -31,7 +32,7 @@ jobs: | |||
# Docker restricts io_uring by default, running unconfined then. | |||
options: --security-opt seccomp=unconfined | |||
steps: | |||
- run: apk add -U git | |||
- run: apk add -U git -U github-cli |
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 second -U
is redundant.
build.zig
Outdated
.tracer_backend = .none, | ||
.hash_log_mode = .none, | ||
}); | ||
const tigerbeetle_past = download_release(b, "latest", options.target, options.mode); |
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.
We do use tigerbeetle_past
above for the same thing, but for multiversioning naming, I think previous
works better when referring to a previous release.
past
internally refers to the non-current releases within a multiversion release.
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.
ah, I got confused thinking that _past
is the right name here, thanks!
} | ||
// When we fetch llvm-objcopy in build.zig, there isn't an easy way to mark it as | ||
// executable, so do it here. | ||
try shell.file_make_executable(cli_args.llvm_objcopy); |
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.
Could this be done with a system command chmod +x
in build_tigerbeetle_executable_get_objcopy
? I'm not sure if that's better... But it feels weird to have a build step that gets you the binary and you then need to make sure it's executable outside of that.
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 current solution is better than chmod
, because system dependencies feel like a worse problem to me. But yeah, this is pretty hacky.
The proper way to do that would be to write a custom chmod
ing step, but that seems relatively large amount of code for little gain, given that the end-game is to not use vendored objcopy at all.
I'll write the custom step if you think thats worthwhile though!
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'll write the custom step if you think thats worthwhile though!
I think it's OK as-is for now! We do want to get rid of that step anyhow at some point. (and if someone else tries to use it in the meantime, figuring out the error will hopefully discourage them 😄.)
src/integration_tests.zig
Outdated
// Against this upgrading cluster, we are running a benchmark load and check that it finishes | ||
// with zero status. |
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.
// Against this upgrading cluster, we are running a benchmark load and check that it finishes | |
// with zero status. | |
// Against this upgrading cluster, we are running a benchmark load and checking that it finishes | |
// with a zero status. |
src/integration_tests.zig
Outdated
} | ||
} | ||
|
||
// context.shell.cwd.deleteTree(context.tmp) catch {}; |
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.
Do we want to clear our tmp 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.
🤦
Two motivations here: - to keep integration tests as close to the real world as possible - to allow integration-testing of upgrades!
dcc0402
to
e59bbff
Compare
Looks like some merge conflicts with |
e59bbff
to
cccb185
Compare
should be good now! |
Adds a real multiversion binary to our integration tests, and implements a simplistic upgrade test.
Ideally, this should be moved to our upcoming system-tests, but let's have at least a smoke test in the meantime: #2270