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

Only pass background exceptions when we #shouldPassBackgroundFailures (fixes #17813) #17814

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

Conversation

daniels220
Copy link
Contributor

Normally during test cleanup we will terminate any suspended background processes, so they will not actually get to resume and hit the #pass. However if the error was raised in an #ensure: block, the background process will finish executing the handler even when sent #terminate, so we need to avoid passing the exception and opening a debugger.

I think this is correct—certainly the behavior of passBackgroundFailures, e.g. when a halt is encountered, should be unaffected as that explicitly causes shouldPassBackgroundFailures to return true before resuming anything. But I'd appreciate another set of eyes on it, and if I should be checking a different condition, or a new condition variable needs to be added, that's possible too.

…fixes pharo-project#17813)

Normally during test cleanup we will terminate any suspended background processes, so they will not actually get to resume and hit the #pass. However if the error was raised in an #ensure: block, the background process will finish executing the handler even when sent #terminate, so we need to avoid passing the exception and opening a debugger.
@daniels220
Copy link
Contributor Author

The CI failure looks like it's just in "Archive JUnit-formatted test results" and no tests actually failed?

@jecisc
Copy link
Member

jecisc commented Mar 7, 2025

This seems OK to me but this part of the system is pretty obscure to me. If someone else can review I'd appreciate it.
If there is no review in the following week, I'll see to merge it

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.

None yet

2 participants