Test: combine gdm tests into one file#8543
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request consolidates several GDM test files into a single module, which is a good step towards simplifying test management. I've reviewed the changes and found a few opportunities to improve the code quality and maintainability of the new combined test file by addressing some code duplication and a potential cause of test flakiness. My specific comments are in the review.
|
The tests look good to me, but I'd recommend you to read Gemini's feedback and check CI failures |
d2bd24b to
20ce25c
Compare
| @pytest.mark.topology(KnownTopology.BareIPA) | ||
| @pytest.mark.builtwith(client="gdm") | ||
| def test_gdm__smartcard_login_with_certs_and_passkey(client: Client, ipa: IPA): | ||
| """ |
There was a problem hiding this comment.
I think all the test docstrings can be simplified and really just describe what the test is supposed to do at a high level", the detail will be in the code. For this test, I don't understand why we have a second set of key/certs and why that use case is important.
Combining all the GDM tests into a single test module to simplify management of these specific tests. Refactoring setup helper functions and adding xidp one. Marking critical tests as well. Removing some unnecessary comments and adding blank lines to make setup steps match code. Renaming some test cases to make purpose more clear.
20ce25c to
5f2939a
Compare
danlavu
left a comment
danlavu
left a comment
There was a problem hiding this comment.
After talking to Scott, the docstring updates are out of scope, but everything else looks good, thanks Scott.
3 participants
Combining all the GDM tests into a single test module to simplify management of these specific tests.
Marking critical tests as well.