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

tests: Add feature presence test suite #7597

Closed
wants to merge 1 commit into from

Conversation

jakub-vavra-cz
Copy link
Contributor

We need to check that features are present where expected.

We need to check that features are present where expected.
@ikerexxe ikerexxe self-assigned this Oct 25, 2024
@ikerexxe ikerexxe self-requested a review October 25, 2024 07:45
@ikerexxe
Copy link
Contributor

Please note my comment at SSSD/sssd-test-framework#133 (comment)

@jakub-vavra-cz
Copy link
Contributor Author

Please note my comment at SSSD/sssd-test-framework#133 (comment)

Hi @ikerexxe ,
test should test one thing (one feature presence).
If I put everything in one test it would be:

  1. Too big and unreadable.
  2. fails on first failed check without showing what is fine/passed.
  3. Un-maintainable if we do not keep the suite the same across all branches.

@jakub-vavra-cz jakub-vavra-cz marked this pull request as ready for review November 6, 2024 10:35
Copy link
Contributor

@shridhargadekar shridhargadekar left a comment

Choose a reason for hiding this comment

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

A suggestion: Could we manually verify the package info? These changes don't need testing in henceforth.

@shridhargadekar shridhargadekar self-assigned this Nov 6, 2024
@ikerexxe
Copy link
Contributor

ikerexxe commented Nov 8, 2024

Hi @ikerexxe , test should test one thing (one feature presence). If I put everything in one test it would be:

1. Too big and unreadable.

2. fails on first failed check without showing what is fine/passed.

3. Un-maintainable if we do not keep the suite the same across all branches.

You can use parametrization to avoid having a very long test. After all, if you look at the tests you have written they all do the same thing: they check the version of sssd and check that the functionality is (or is not) available in that specific version.
With parametrization the lines of code would be the same, only that the information referring to the functionality (sssd version and feature) would be defined in the parametrization itself. This way we reduce the lines of code to maintain and the checking of each feature is done separately (if a feature fails it does not affect the execution of the rest).

@jakub-vavra-cz
Copy link
Contributor Author

Hi @ikerexxe , test should test one thing (one feature presence). If I put everything in one test it would be:

1. Too big and unreadable.

2. fails on first failed check without showing what is fine/passed.

3. Un-maintainable if we do not keep the suite the same across all branches.

You can use parametrization to avoid having a very long test. After all, if you look at the tests you have written they all do the same thing: they check the version of sssd and check that the functionality is (or is not) available in that specific version. With parametrization the lines of code would be the same, only that the information referring to the functionality (sssd version and feature) would be defined in the parametrization itself. This way we reduce the lines of code to maintain and the checking of each feature is done separately (if a feature fails it does not affect the execution of the rest).

Hi @ikerexxe I am pondering how to parametrize that apart from having something like dictionary with feature as key and Some kind of list with distribution+minversion+maxversion. Can You elaborate/show me some snippet?

@jakub-vavra-cz
Copy link
Contributor Author

Looks test for "files-provider" does not work properly on centos 9 as it seems to be present on sssd 2.10 there.
For F39, F40 there seems to be issue with "non-privileged" that is not present on 2.10 there.

I guess these tests need tweaking if this is the intended result.

@ikerexxe
Copy link
Contributor

Hi @ikerexxe I am pondering how to parametrize that apart from having something like dictionary with feature as key and Some kind of list with distribution+minversion+maxversion. Can You elaborate/show me some snippet?

@jakub-vavra-cz that's exactly what I had in mind. If you want to be more comprehensive and take into account the passkey functionality then you should also add the distribution and the version from which it is available.

@ikerexxe
Copy link
Contributor

ikerexxe commented Dec 3, 2024

@jakub-vavra-cz #7740 is my proposal to take care of feature detection. Take a look at it and tell me what you think

@jakub-vavra-cz
Copy link
Contributor Author

Closing in favour of parametrized implementation in: #7740

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

Successfully merging this pull request may close these issues.

4 participants