Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

benchmarks/CI: make benchmarks compile and build them in CI #853

Merged

Conversation

piodul
Copy link
Collaborator

@piodul piodul commented Oct 20, 2023

The recent fix that changed the types::write_short function to use u16 instead of i16 broke one of our benchmarks - it tried to call that function with a negative number. The benchmarks aren't compiled in our CI, so it didn't catch it.

In order to prevent it from happening in the future, the rust workflow now uses the --all-targets flag to build and clippy-check the benchmarks as well.

Pre-review checklist

  • I have split my patch into logically separate commits.
  • All commit messages clearly explain what they change and why.
  • I added relevant tests for new features and bug fixes.
  • All commits compile, pass static checks and pass test.
  • PR description sums up the changes and reasons why they should be introduced.
  • I have provided docstrings for the public items that I want to introduce.
  • I have adjusted the documentation in ./docs/source/.
  • I added appropriate Fixes: annotations to PR description.

The recent fix that changed the `types::write_short` function to use u16
instead of i16 broke one of our benchmarks - it tried to call that
function with a negative number. The benchmarks aren't compiled in our
CI, so it didn't catch it.

Fix the issue by using `u16::MAX` instead of `-1`.
Our previous combination of the flags didn't build or check the
benchmarks at all. In order to prevent this from happening in the
future, adjust the `cargo check`, `cargo clippy` and `cargo build`
invocations to use the `--all-targets` flag.
@piodul piodul requested a review from Lorak-mmk October 20, 2023 12:34
@piodul
Copy link
Collaborator Author

piodul commented Oct 26, 2023

@Lorak-mmk review ping

@Lorak-mmk Lorak-mmk merged commit 5830269 into scylladb:main Oct 26, 2023
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants