Skip to content

Use test_assert for debug_asserts that are used for failing tests #5318

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ksolana
Copy link

@ksolana ksolana commented Mar 16, 2025

Problem

These tests fail in release mode because they use debug_assert to trigger a panic.

Summary of Changes

Add a test_assert that only triggers in test and not dependent on debug/release.

Fixes: #5316

Copy link

@brooksprumo brooksprumo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm personally not a fan of wholesale replacing debug_assert! with a test-only assert. Maybe the test should be rewritten? Or maybe the functions being tested need to be rewritten/refactored for testability.

Also, we don't test with release builds, so I'm not in agreement with the problem statement of this PR.

@ksolana
Copy link
Author

ksolana commented Mar 17, 2025

I'm personally not a fan of wholesale replacing debug_assert! with a test-only assert. Maybe the test should be rewritten? Or maybe the functions being tested need to be rewritten/refactored for testability.

sounds reasonable. I can rewrite those tests to remove debug_asserts.

Also, we don't test with release builds, so I'm not in agreement with the problem statement of this PR.

that was the argument(that we should be testing in release builds as well) raised in https://discord.com/channels/428295358100013066/838890116386521088

@ksolana
Copy link
Author

ksolana commented Mar 22, 2025

#5361 handles the div better.

@roryharr
Copy link

#5361 handles the div better.

Merged 5361 today

@alexpyattaev
Copy link

If these are tests, what is wrong with just using assert! ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failures in release mode
4 participants