Skip to content
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

feat: run unit tests with --release flag #173

Merged
merged 1 commit into from
Dec 9, 2024

Conversation

ShadowCurse
Copy link
Collaborator

@ShadowCurse ShadowCurse commented Dec 9, 2024

Summary of the PR

Rust does optimizations which can cause havoc in --release mode, so add another set of unit tests
with all optimizations enabled.

See: rust-vmm/kvm#298

Requirements

Before submitting your PR, please make sure you addressed the following
requirements:

  • All commits in this PR have Signed-Off-By trailers (with
    git commit -s), and the commit message has max 60 characters for the
    summary and max 75 characters for each description line.
  • All added/changed functionality has a corresponding unit/integration
    test.
  • All added/changed public-facing functionality has entries in the "Upcoming
    Release" section of CHANGELOG.md (if no such section exists, please create one).
  • Any newly added unsafe code is properly documented.

roypat
roypat previously approved these changes Dec 9, 2024
@stefano-garzarella
Copy link
Member

The changes LGTM, but I have a couple of questions:

  1. should we run unit tests for both debug and release? (not sure if for the same reason, running tests only with optimizations, can hide some issues)
  2. should we do the same for coverage? (in integration_tests/test_coverage.py we call cargo llvm-cov test without --release)

@ShadowCurse
Copy link
Collaborator Author

Added unit tests with debug build back.
Regarding coverage, I would assume this is not impacted by the optimization level, so no reason to add it there.

@stefano-garzarella
Copy link
Member

Added unit tests with debug build back.

Can you also update commit and PR title/description?

Regarding coverage, I would assume this is not impacted by the optimization level, so no reason to add it there.

I see, definitely not to be fixed in this PR, but your changes made me wonder if it makes sense to calculate coverage by considering, for example, code that is not used in release (maybe we don't even have it, but it occurred to me). I'll open an issue to discusse that

Rust does optimizations which can cause havoc
in `--release` mode, so add another set of unit tests
with all optimizations enabled.

Signed-off-by: Egor Lazarchuk <[email protected]>
@stefano-garzarella stefano-garzarella merged commit c04433e into rust-vmm:main Dec 9, 2024
3 checks passed
@ShadowCurse ShadowCurse deleted the release_unittests branch December 9, 2024 15:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants