-
Notifications
You must be signed in to change notification settings - Fork 247
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: Porting sss_override test suite #6717
Conversation
bf8e95f
to
4a4f18b
Compare
============================= test session starts ============================== tests/test_local_overrides.py::test_local_overrides__user (ad) ======================== 12 passed in 71.55s (0:01:11) ========================= |
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.
A few changes, suggestions, and questions.
6ed82d0
to
18a3124
Compare
8afbe1d
to
9daee9a
Compare
465f6dc
to
dd01d50
Compare
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.
The mypy errors and the fedora system test failures are due to the dependent PR here:
I'm expecting once that is merged this will go green or much more of it will anyway.
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.
Hi Dan, thank you. I think this is a good start, there are just few things that I wanted to explain. Please, see comments inside.
sss_override API in sssd-test-framework is now merged, please, let me know when this PR is ready for next round of review |
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.
Hi Dan, excellent work. I have some comments to improve readability, all of them applies to the whole code (and future tests you will write). But no functional changes.
You also need to add :requirement:
metadata and it would be good to provide assert message to places when failure may not be clear (i.e. assert result is not None, "User was not found"
0d60024
to
7fd1502
Compare
7fd1502
to
317c030
Compare
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.
Hi, after these two changes, ack.
317c030
to
19a40da
Compare
03ac2a6
to
35f4583
Compare
35f4583
to
1895a5f
Compare
@sidecontrol Ack. Is there any reason why this should not go to sssd-2-9? It does not test any new functionality, it just ports the tests so imho we can backport it so we run these tests again 2.9.* builds in downstream? |
I was thinking, no reason, added 2-7 and 2-8 |
2.7 does not have system tests, lets use 2.8, 2.9 and master. |
No description provided.