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

feat: report image info without StSystemReporter #646

Merged

Conversation

theseion
Copy link
Collaborator

StSystemReporter was removed in Pharo 13.

`StSystemReporter` was removed in Pharo 13.
@theseion theseion requested a review from fniephaus June 15, 2024 05:54
The default seed for `Random` is clock based, meaning that instantiating
`Random` multiple times will produce identical values if the clock
doesn't change. Use the global shared generator instead for
`#newTravisID`.
@theseion
Copy link
Collaborator Author

@estebanlm, @guillep Here's such an instance where the behavior of Pharo64-13 differs from Pharo64-alpha: https://github.com/hpi-swa/smalltalkCI/actions/runs/9525945437/job/26260957214?pr=646#step:4:236.

The method simply checks wether generating two random values twice produces a different identifier, equivalent of:

(ByteArray with: 255 atRandom with: 255 atRandom) hex ~= (ByteArray with: 255 atRandom with: 255 atRandom)

For some very weird reason, Random>>#nextInteger: produces the same value multiple times. At first, I thought it was a timing issue, so I fixed the use of Random in #newTravisID, but the test still fails. Interestingly, the test only fails on the CI, not on my machine. I suspect, that it's an issue that depends on OS and VM combination (can't test that on my machine).

@demarey
Copy link
Contributor

demarey commented Jun 17, 2024

The Random problem in Pharo is now fixed: pharo-project/pharo#16780.
I also proposed a PR to fi fix the problem of the system reporter in P13: #649

@theseion
Copy link
Collaborator Author

@fniephaus good to go

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, thanks!

@fniephaus fniephaus merged commit c37fbbf into hpi-swa:master Jun 17, 2024
78 checks passed
@theseion theseion deleted the print-image-info-without-system-reporter branch June 17, 2024 18:09
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.

3 participants