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 flaky tests by disregarding order of mc-reco links #130

Merged
merged 3 commits into from
Oct 4, 2024

Conversation

tmadlener
Copy link
Contributor

@tmadlener tmadlener commented Oct 2, 2024

BEGINRELEASENOTES

  • Make MC-Reco links comparisons pass even if they are not the same order in EDM4hep and Delphes as long as all the links are present.

ENDRELEASENOTES

- [ ] Needs key4hep/key4hep-actions#7

As already stated in the inline comment: Delphes seems to write these in different orders even with the same random seed. It seems to only happen during writing of the Delphes file, as the EDM4hep files that come out of the converter are the same. I have done a diff <(podio-dump -d -e <offending-event> <run_1.root>) <(podio-dump -d -e <offending-event> <run-2.root>) and that yields no differences in the files apart from the obviously different file names.

Given all that, I think this is the most reasonable fix we can make.

Fixes #129

@tmadlener tmadlener changed the title [WIP] Try to see if running repeatedly fixes flaky test Fix flaky tests by disregarding order of mc-reco links Oct 4, 2024
@tmadlener
Copy link
Contributor Author

I didn't observe any failures locally with this in 30 repetitions. Given previous experience in #129 I would have expected around 10, so I think this at least fixes the observed failure.

@tmadlener
Copy link
Contributor Author

Merging this, then rebasing #128 to get a tag done for the next Key4hep release, unless someone complains in the next hour.

@tmadlener tmadlener merged commit 79f25af into key4hep:main Oct 4, 2024
5 of 7 checks passed
@tmadlener tmadlener deleted the flaky-test-tmp-fix branch October 4, 2024 13:52
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.

DelphesRootReader_ee_91gev test failing
2 participants