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

Make sure that files with the complete datamodel can be read #361

Merged
merged 18 commits into from
Sep 16, 2024

Conversation

tmadlener
Copy link
Contributor

@tmadlener tmadlener commented Sep 10, 2024

BEGINRELEASENOTES

  • Refactor createEDM4hepFile.py script to facilitate testing. Apart from the obviously different values in the objects, the overall structure is almost unchanged for the output file.
    • The MCVertexRecoParticleLinkCollection has been renamed to the VertexRecoParticleLinkCollection
    • The subdetectorHoleNumbers are also populated for Tracks.

ENDRELEASENOTES

I have refactored the createEDM4hepFile.py script to effectively reset the counter to 0 for every datatype in order to make testing a bit easier, because we won't have global changes in expected values in case some data members are added or removed. With this we can use this script to generate an EDM4hep file for every tag we have and store it somewhere (probably EOS, the same way we do it for podio) and download them on-the-fly for testing to be able to check that we don't accidentally break backwards compatibility with changes to the datamodel.

This is the first part of #358 without the backwards compatibility parts yet, to disentangle the two and to make it possible to tag this part first before adding backwards compatibility tests. In this way the latter will have a clearly reproducible path to regenerating input files if necessary.

test/backwards_compat/conftest.py Outdated Show resolved Hide resolved
test/backwards_compat/test_EDM4hepFile.py Outdated Show resolved Hide resolved
test/backwards_compat/CMakeLists.txt Outdated Show resolved Hide resolved
test/test_EDM4hepFile.py Outdated Show resolved Hide resolved
test/test_EDM4hepFile.py Outdated Show resolved Hide resolved
Copy link
Contributor

@jmcarcell jmcarcell left a comment

Choose a reason for hiding this comment

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

For this PR I don't have any more comments, I'll think about the other PR related to backwards compatibility.

scripts/createEDM4hepFile.py Outdated Show resolved Hide resolved
test/test_EDM4hepFile.py Outdated Show resolved Hide resolved
test/test_EDM4hepFile.py Outdated Show resolved Hide resolved
Comment on lines +141 to +143
# With the current version of cppyy (from ROOT 6.30.06)
# it's not possible to initialize an std::array from a list
# In future versions (works with 6.32.02) it will be possible
Copy link
Contributor

Choose a reason for hiding this comment

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

Just to confirm, the cppyy version coming with ROOT 6.30.06 is 1.6.2 and with ROOT 6.32.02 is 3.1.2, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes.

@tmadlener
Copy link
Contributor Author

It looks like GaudiTesting produces some <DartMeasurement ... XML output that contains the complete test file as a string, which in turn matches the FAIL_REGULAR_EXPRESSION from the test environment. I have disabled this matching for the new tests. Hopefully now everything that we expect to pass should also pass.

@tmadlener tmadlener merged commit 8e0c1b0 into key4hep:main Sep 16, 2024
7 of 10 checks passed
@tmadlener tmadlener deleted the read-back-file branch September 16, 2024 14:34
@jmcarcell
Copy link
Contributor

Hmm locally I get that is loading GaudiTesting but the tests pass. The last change here doesn't change anything for me, the tests always pass. To remove GaudiTesting from the plugins one can add -p no:gaudi_fixtures -p no:collect_for_ctest -p no:ctest_measurements_reporter to the configuration or to the command. Maybe it's worth to add it but there is some ongoing work for tests right now in Gaudi so these things may change.

@tmadlener
Copy link
Contributor Author

The tests were always passing. However, the output contained things like getEnergyError (from the test code) which trigger this regular expression

FAIL_REGULAR_EXPRESSION "[^a-z]Error;ERROR;error;Failed"

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