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: adding group and importance markers #6902

Closed
wants to merge 1 commit into from

Conversation

danlavu
Copy link

@danlavu danlavu commented Aug 25, 2023

No description provided.

src/tests/system/pytest.ini Outdated Show resolved Hide resolved
@danlavu danlavu force-pushed the importance branch 2 times, most recently from 6ad02be to 898df05 Compare August 31, 2023 00:09
@jakub-vavra-cz
Copy link
Contributor

Please do not add these to high/critical:
FAILED ../sssd/src/tests/system/tests/test_autofs.py::test_autofs__propagate_offline__single_domain (ad)
FAILED ../sssd/src/tests/system/tests/test_sudo.py::test_sudo__sudonotbefore_shorttime (ldap)
I think that there was a bug in sudo rules that affects the test (but bug reference is missing from the failing test for some reason).

As for test_autofs__propagate_offline__single_domain, it just blocks ldap port but does nothing about other AD ports and it does not look correct to me as it is for AD provider.

I would generally not consider tests for backends offline as critical apart from some specific ones. (like caching works when the backend goes offline).

The critical set should contain only the most important tests (smoke tests) to represent the the important features.

@danlavu
Copy link
Author

danlavu commented Aug 31, 2023

@jakub-vavra-cz Ack, removed from critical/high and moved to medium/low.

Copy link
Contributor

@jakub-vavra-cz jakub-vavra-cz left a comment

Choose a reason for hiding this comment

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

LGTM.

Copy link
Member

@pbrezina pbrezina left a comment

Choose a reason for hiding this comment

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

@jakub-vavra-cz we need to create some guidance how to assign the importance level and how does it map to downstream test execution. Can you update https://tests.sssd.io/en/latest/marks.html#pytest-mark-tier?

From my point of view, if there is a failure, we can not release new version. So every test counts. In this sense, every test is critical. So having guidance and use case documented would help a lot.

src/tests/system/pytest.ini Outdated Show resolved Hide resolved
src/tests/system/pytest.ini Outdated Show resolved Hide resolved
src/tests/system/pytest.ini Outdated Show resolved Hide resolved
src/tests/system/pytest.ini Outdated Show resolved Hide resolved
@pbrezina
Copy link
Member

pbrezina commented Sep 4, 2023

@sidecontrol I agree with the markers, but not with the documentation. It needs to be clearly documented how we want to use them - like schema and cache for example.

pytest.ini is only suitable for short description, https://tests.sssd.io/en/latest/marks.html#additional-markers is a good place for a thorough guidance.

If you agree, I think we should add comment to pytest.ini # see explanation of the markers at xyz and even remove the short description as it just repeates the marker name but does not provide more information. Then I think we can merge this one a you will follow with pull requests to sssd-test-framework/docs

@danlavu
Copy link
Author

danlavu commented Sep 6, 2023

@pbrezina Yup, I'll create another pull request with the doc explaining the markers.

@pbrezina
Copy link
Member

pbrezina commented Sep 8, 2023

I marked this as blocked, because I think that we need the documentation first. Because until we have documentation, there will still be new tests that may miss this and you will need to update them here.

@danlavu
Copy link
Author

danlavu commented Sep 12, 2023

@pbrezina

SSSD/sssd-test-framework#43

doc update.

Copy link
Contributor

@patriki01 patriki01 left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. I approve.

@jakub-vavra-cz
Copy link
Contributor

Pushed PR: #6902

  • master
    • f05d4ec - tests: adding group and importance markers
  • sssd-2-9
    • 674ee26 - tests: adding group and importance markers

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