Skip to content

Conversation

juj
Copy link
Collaborator

@juj juj commented Sep 19, 2025

Revise stack overflow and abort checks in the core suite to allow testing stack overflow tests also in minimal0 suite.

Checking for the presence of the "Aborted(" prefix does not seem essential to any of these tests.

Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

While not essential I think check for the Aborted( prefix does allow for fewer false positives.

What about just adding the Aborted( prefix to MINIMAL_RUNTIME?

@juj
Copy link
Collaborator Author

juj commented Sep 19, 2025

While not essential I think check for the Aborted( prefix does allow for fewer false positives.

I don't think any of the prints here would have a big risk for a false positive.

What about just adding the Aborted( prefix to MINIMAL_RUNTIME?

That is the kind of cruft that MINIMAL_RUNTIME wanted to avoid.. there is no essential reason for that except for debugging purposes, and those things pile up, like the initial cleanup from regular runtime -> minimal runtime showed.

@sbc100
Copy link
Collaborator

sbc100 commented Sep 19, 2025

While not essential I think check for the Aborted( prefix does allow for fewer false positives.

I don't think any of the prints here would have a big risk for a false positive.

Sure, I guess. I could imagine we might have test that did something like printf("Running stack overflow test").. but I guess the risk is fairly low.

It also makes the tests a little less clear in what they are testing. Without this change I can clearly see that I'm expecting the test for fail with an abort call.. after this it less obvious the reader what is being tested for.

We could do something gross like abort_message('stack_overflow) which would conditionally add the prefix.. or maybe we could have the test output checker strip the Aborted( prefix when MINIMAL_RUNTIME is set.

Neither of those options seem that great either though :)

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.

2 participants