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

test for ClassVersion backwards compatibility #39008

Open
jpata opened this issue Aug 9, 2022 · 5 comments
Open

test for ClassVersion backwards compatibility #39008

jpata opened this issue Aug 9, 2022 · 5 comments

Comments

@jpata
Copy link
Contributor

jpata commented Aug 9, 2022

In #38870 (comment) we missed keeping the old ClassVersion, which has since been readded in #38992.

It might be useful if this bug was tested for: either at compile time, statically, or via some runtime test.

@jpata
Copy link
Contributor Author

jpata commented Aug 9, 2022

assign core

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 9, 2022

New categories assigned: core

@Dr15Jones,@smuzaffar,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

cmsbuild commented Aug 9, 2022

A new Issue was created by @jpata Joosep Pata.

@Dr15Jones, @perrotta, @dpiparo, @rappoccio, @makortel, @smuzaffar, @qliphy can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@makortel
Copy link
Contributor

makortel commented Aug 9, 2022

Adding additional comment from #38986 (comment)

Version numbers 0, 1, and 2 have special meaning to ROOT and should not be used as version numbers. By convention, CMS will use 3 as the initial version number of a class which has never been stored before.

Something that could as well be caught via a test...

@makortel
Copy link
Contributor

makortel commented Aug 9, 2022

So we'd need to somehow figure out if a a change in classes_def.xml kept all previous class versions, with an ability to ignore that in case a "break" is really needed (which should be rare).

I'd imagine a check in the PR test infrastructure to be sufficient. My first thought would be to write a python script that would parse two classes_def.xml files as an input (old and new, possibly reading one of them from stdin so it could be given with git show <commit-id>:<path> without having to check the actual file out), and raise an error if

  • any old class version is removed, or
  • any new class version is less than 3

It would collect all possible errors from the two files before reporting. Failures would be reported in the PR test summary message in the PR. I'm not sure if failures should lead to -1 for the PR or not. Given that cases where the "failure" is expected should be rare, maybe it should lead to failing the PR tests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants