FORM Test for ROOT Schema Evolution

[aolivier23]

FORM should currently support ROOT's schema evolution facilities for adapting to relatively simple changes to data products. Verifying this is useful for FORM development because RNTuple exposes an interface that bypasses schema evolution. This test builds on the Track_Start data product used in existing FORM tests to verify that FORM's TTree container successfully supports ROOT's schema evolution. Future FORM container technologies will be accompanied by an extension of this test.

[aolivier23]

@phlexbot format

[github-actions]

No automatic format fixes were necessary.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

@@            Coverage Diff             @@
##             main     #388      +/-   ##
==========================================
- Coverage   86.45%   84.45%   -2.00%     
==========================================
  Files         119      126       +7     
  Lines        2399     3526    +1127     
  Branches      387      650     +263     
==========================================
+ Hits         2074     2978     +904     
- Misses        207      333     +126     
- Partials      118      215      +97     

Flag	Coverage Δ
unittests	84.40% <ø> (-2.06%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 41 files with indirect coverage changes

---

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30457cd...df6d2da. Read the comment docs.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[wwuoneway]

It looks like extra_member is intended to be an independent test. Considering renaming it to something more descriptive of the scenario would be helpful. I noticed some duplicated files between theextra_member folder and the up-level one, that’s fine if the tests are meant to be self-contained, but it may be worth factoring out common pieces in the future.

[wwuoneway]

Should it be #include "test/form/data_products/extra_member/track_start.hpp" here?

[aolivier23]

No, this is intentional. form_root_schema_write_test writes the old, non-extra_member version of the data product to a file. form_root_schema_read_test reads the old product with only the new product's definition. ROOT should automatically fill in the in-memory new product with as much of the old product's data as possible.

[aolivier23]

It looks like extra_member is intended to be an independent test. Considering renaming it to something more descriptive of the scenario would be helpful. I noticed some duplicated files between theextra_member folder and the up-level one, that’s fine if the tests are meant to be self-contained, but it may be worth factoring out common pieces in the future.

You make a good point here, and I'm willing to make it happen. But extra_member/track_start.hpp is part of a test that also uses the regular data_products/track_start.hpp. The class TrackStart is the schema that's evolving in this test, and it's weird but necessary that both exist in the same version of the repository for this test to work.

In contrast, when the situation I'm testing happens in production, someone would change the definition of TrackStart and then try to read back a file written with the old version. We wouldn't normally have two files defining the same TrackStart class in the same version of the repository.

I think we could make a directory with a name like form/tests/data_products_extra_members for the content of form/tests/data_products/extra_members. We could also have two calls to reflex_generate_dictionary in a single shared CMakeLists.txt. But I think the way I committed things, we're less likely to have someone accidentally combine them into one reflex_generate_dictionary and break things in a confusing way. What do you think?

[wwuoneway]

@aolivier23 Thanks for the clarification. I think we can proceed this way for now. I'll make a note for future.

[wwuoneway]

thanks, LGTM

[barnaliy]

This is a well-structured test for an important feature. Just one comment:

The log comparison between write and read tests correctly verifies that existing fields (x, y, z) survive schema evolution. It might also be worth considering whether to include m_index in the operator<< output, just to give explicit visibility into how the new field is defaulted after reading the old product — though this is a minor point and may be outside the scope of what this test is trying to demonstrate.

[aolivier23]

This is a well-structured test for an important feature. Just one comment:

The log comparison between write and read tests correctly verifies that existing fields (x, y, z) survive schema evolution. It might also be worth considering whether to include m_index in the operator<< output, just to give explicit visibility into how the new field is defaulted after reading the old product — though this is a minor point and may be outside the scope of what this test is trying to demonstrate.

That's fair. I'm leaning towards revisiting this when phlex has more to say about schema evolution in the far future. The main situation I want to test for, me misusing RNTuple in the near future, would probably crash the schema evolution reader before we even got to file comparisons.