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

Fix retrieval of package name for a method in Pharo 12 #638

Merged
merged 2 commits into from
Apr 8, 2024

Conversation

jbrichau
Copy link

This PR fixes a crash in Pharo 12 when code coverage reporting is enabled and when methods of a Trait are passed through SCIPharo12CodeCoverage class>>#packageNameForMethod:

Without the fix: https://github.com/SeasideSt/Seaside/actions/runs/8442597789/job/23124347278
With the fix: https://github.com/SeasideSt/Seaside/actions/runs/8482174261

I also notice metadata is removed. Since the changes are in a Pharo-specific package, I left them in. If you want me to revert those changes and keep metadata, let me know.

Johan Brichau added 2 commits March 29, 2024 15:40
… when methods of a Trait are passed through SCIPharo12CodeCoverage class>>#packageNameForMethod:
@jbrichau
Copy link
Author

Since removing the method metadata breaks loading for platforms as old as Pharo 3 (even though the package is not for Pharo 3), I reverted the metadata changes.

I would recommend to drop those old Pharo versions for the future ;-)

@jbrichau
Copy link
Author

The build failed because of a download failure. I cannot restart the build.

@jbrichau
Copy link
Author

jbrichau commented Apr 7, 2024

@fniephaus I cannot re-run the action jobs on this PR. Could you? All the runs should be green again but the previous built failed because of a download failure.

@fniephaus
Copy link
Member

sure, no problem

@jbrichau
Copy link
Author

jbrichau commented Apr 8, 2024

It looks like this is working well.
It fixes the issue I have running code coverage in Seaside on Pharo 12 test runs.

Copy link
Member

@fniephaus fniephaus left a comment

Choose a reason for hiding this comment

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

LGTM, simpler is always better!

@fniephaus fniephaus merged commit 8e603d8 into hpi-swa:master Apr 8, 2024
79 checks passed
jbrichau pushed a commit to SeasideSt/Seaside that referenced this pull request Apr 19, 2024
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