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

Do not disable system announcements during rpackage tests #13298

Conversation

jecisc
Copy link
Member

@jecisc jecisc commented Apr 5, 2023

RPackage tests were disable the system announcer but this causes multiple problems such as:

  • Automatic transformation was happening but no tool were aware of it (Calypso did not update the source, Iceberg did not catch the change...)
  • Coding in debugger was dangerous because no system announcements were happening
  • The system was vulnerable during the execution of those tests

So I propose to just keep the system announcer while testing RPackage

Also I shipped some deprecation rewrites now that Iceberg notice them :)

RPackage tests were disable the system announcer but this causes multiple problems such as:
- Automatic transformation was happening but no tool were aware of it (Calypso did not update the source, Iceberg did not catch the change...)
- Coding in debugger was dangerous because no system announcements were happening 
- The system was vulnerable during the execution of those tests

So I propose to just keep the system announcer while testing RPackage
Copy link
Contributor

@privat privat left a comment

Choose a reason for hiding this comment

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

This is interesting. We want announcements to not necessarily be global (especially for tests), but in practice it's hard to achieve.

@jecisc
Copy link
Member Author

jecisc commented Apr 5, 2023

Yes!

The problem here is that the way to not do system announcement impact everything.

Ideally, you want an announcer specific for the code generated by your new package organizer and keep the global one for the code of the tests itself.

Maybe a solution would be to make RPackageOrganizer hold an announcer and use this one. Default RPackageOrganizer would use the system announcer and the test would have their own instance that is not the global one?

@jecisc
Copy link
Member Author

jecisc commented Apr 5, 2023

Let's note that for now this is hard to do because it's SystemOrganizer that launches the announcements. But when RPackageOrganizer will replace SystemOrganizer, it should be easy not to use global announcer everywhere :)

@jecisc
Copy link
Member Author

jecisc commented Apr 5, 2023

testOnlyWeakSubscriptions fails and seems to be related.

@jecisc jecisc added the Status: Need more work The issue is nearly ready. Waiting some last bits. label Apr 5, 2023
@jecisc
Copy link
Member Author

jecisc commented Apr 6, 2023

RPackage tests where using strong subscription to announcements. I added a fix to make all subscriptions weak

@MarcusDenker MarcusDenker merged commit 2c3f051 into pharo-project:Pharo12 Apr 6, 2023
@jecisc jecisc deleted the do-not-disable-system-announcements-during-rpackage-tests branch April 6, 2023 15:06
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.

3 participants