Skip to content

Add more comments to libc-fs-with-isolation test #4322

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

Merged
merged 1 commit into from
May 18, 2025

Conversation

tiif
Copy link
Contributor

@tiif tiif commented May 16, 2025

Add comments to explain why we test syscalls that need disable isolation mode in isolation mode.

@tiif
Copy link
Contributor Author

tiif commented May 16, 2025

@rustbot ready

@rustbot rustbot added the S-waiting-on-review Status: Waiting for a review to complete label May 16, 2025
@RalfJung
Copy link
Member

RalfJung commented May 17, 2025

Looking at this again when I am more awake ;)

These tests do not "require disable-isolation". These tests test what happens when you have isolation enabled, and set -Zmiri-isolation-error=warn-nobacktrace. It's important to test that. So we definitely do not want to move these tests anywhere else -- we want to test the behavior of all our APIs both with and without isolation.

@RalfJung
Copy link
Member

@rustbot author

@rustbot rustbot removed the S-waiting-on-review Status: Waiting for a review to complete label May 17, 2025
@rustbot
Copy link
Collaborator

rustbot commented May 17, 2025

Reminder, once the PR becomes ready for a review, use @rustbot ready.

@rustbot rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label May 17, 2025
@tiif
Copy link
Contributor Author

tiif commented May 17, 2025

These tests do not "require disable-isolation". These tests test what happens when you have isolation enabled, and set -Zmiri-isolation-error=warn-nobacktrace. It's important to test that. So we definitely do not want to move these tests anywhere else -- we want to test the behavior of all our APIs both with and without isolation.

Ahh this makes sense, thanks for explaining! I will move the test back and add this to comment later.

@tiif tiif changed the title Run some tests in disable-isolation Add more comments to libc-fs-with-isolation test May 17, 2025
@tiif
Copy link
Contributor Author

tiif commented May 17, 2025

Alright, I just add more comment there

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels May 17, 2025
@RalfJung
Copy link
Member

This looks great, thanks! After resolving the last nit, please squash the commits using ./miri squash. Then write @rustbot ready after you pushed the squashed PR.

@rustbot author

@rustbot rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels May 18, 2025
@tiif
Copy link
Contributor Author

tiif commented May 18, 2025

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels May 18, 2025
@RalfJung RalfJung enabled auto-merge May 18, 2025 11:46
@RalfJung RalfJung added this pull request to the merge queue May 18, 2025
Merged via the queue into rust-lang:master with commit f1142b5 May 18, 2025
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Waiting for a review to complete
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants