-
Notifications
You must be signed in to change notification settings - Fork 121
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 older versions in new Fedora releases. Closes #28 #91
Conversation
With the latest commit I don't report errors when the command implementation and the handler differ in terms of RHEL vs. Fedora. (Deliberately made as separate commit for easier review, can squash later). The error list is now reduced to 7 errors which seem legitimate:
|
s_impl_version.startswith('RHEL')) or \ | ||
(s_handler_version.startswith('RHEL') and \ | ||
s_impl_version.startswith('F')): | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this block. It introduces the possibility to silent legitimate errors. For example modify handlers/rhel7.py
to contain "autopart': pykickstart.commands.autopart.F20_AutoPart then run the test. The error isn't detected.
Part of the problem is the fact that RHEL7_AutoPart = F21_AutoPart. If we define the class like this
class RHEL7_AutoPart(F21_AutoPart):
pass
then the error is detected even with the skip block in place. Maybe I can add another test for those Fxy_Command = Fxx_Command definitions and update the code where necessary. What do you guys think ?
This test loops over the contents of commandMap and dataMap and tries to find handlers (e.g. Fedora versions) for which the listed commands and data attributes are not the latest possible version defined in the corresponding command.py file. For example if the latest release is Fedora24 and we're testing the handler for Fedora23's "logvol" then we expect the mapping to be F23_LogVol or lover, even if F24_LogVol is present. However if the mapping is F20_LogVol then this is a bug b/c F23_LogVol provides newer implementation which isn't used. The test doesn't report errors for RHEL vs Fedora differences and vice versa.
I've rebase to latest and fixed the identified errors in the last commit. I also fixed a few minor issues along the way. @clumens do you want to merge everything as-is or want me to split out the commits which are not related to this test into a separate PR? |
I'll just merge everything as-is, thanks. |
This test loops over the contents of commandMap and dataMap and tries to find handlers (e.g. Fedora versions) for which the listed commands and data attributes are not the latest possible version defined in the corresponding command.py file.
For example if the latest release is Fedora24 and we're testing the handler for Fedora23's "logvol" then we expect the mapping to be F23_LogVol or lover, even if F24_LogVol is present. However if the mapping is F20_LogVol then this is a bug b/c F23_LogVol provides newer implementation which isn't used. Currently the test fails with 51 errors.
The Fedora vs. newer RHEL are probably false negatives. Then the test correctly identifies the logvol problems in f22.py and some in devel.py.
I've also made some minor fixes and adjustments in the first several commits so I can get this working. The test itself is in the last commit.
@vpodzime, @clumens please comment on this.