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: removing intg/test_confdb.py #7578

Closed
wants to merge 1 commit into from

Conversation

danlavu
Copy link

@danlavu danlavu commented Sep 9, 2024

Dropping these tests as they do not have much impact.

@ikerexxe
Copy link
Contributor

ikerexxe commented Sep 11, 2024

I fail to see where the removed these are covered in test_sssctl.py. I'm not a expecting a 100% match, but something similar at least. Dan can you point me one of the test cases that covers these scenarios?

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.

Could you share the coverage in new testframework?

@danlavu
Copy link
Author

danlavu commented Sep 12, 2024

@ikerexxe

The description is inaccurate, so these are the three tests, the first two are tested just by running tests, the last one I felt we can drop these tests as they really do not add much value.

test_domains__enabled - Test that SSSD starts with explicitly configured domain. missing domain=test
test_domains__domains - Test that SSSD starts without domains option. enabled=true as a default value
test_domains__empty - Test that SSSD fails without any domain enabled enabled=false*

We can add them still, these would be marked integration and set to a low priority? What would you prefer?

@danlavu danlavu force-pushed the tests-rm-intg-confdb branch from 09a875e to 6543895 Compare September 12, 2024 14:24
@danlavu
Copy link
Author

danlavu commented Sep 12, 2024

I went ahead and quickly wrote the tests and added them to test_files.py. They don't offer much.

@ikerexxe
Copy link
Contributor

Now you can understand our confusion when we were reviewing the PR. I'm fine with both transforming the tests and with removing them.

As a side note, if you want to remove test_confdb.py, then you'll need to remove its reference from src/tests/intg/Makefile.am. I think that's the only place where the file is mentioned in our build infrastructure. That should make CI happy.

@danlavu danlavu force-pushed the tests-rm-intg-confdb branch from 6543895 to 2ca5528 Compare September 12, 2024 15:59
@danlavu
Copy link
Author

danlavu commented Sep 12, 2024

@ikerexxe I understand entirely, and I'm sorry for generalizing; there are many things to track as we're doing this test transformation.

I updated the Makefile on all my PRs. Thank you.

These test are covered test_sssctl.py
@danlavu danlavu force-pushed the tests-rm-intg-confdb branch from 2ca5528 to 9c7c39c Compare September 12, 2024 23:19
Copy link
Contributor

@ikerexxe ikerexxe left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you for taking care of it

@alexey-tikhonov alexey-tikhonov added the Ready to push Ready to push label Sep 17, 2024
@alexey-tikhonov
Copy link
Member

Pushed PR: #7578

  • master
    • 97571b1 - tests: removing intg/test_confdb.py
  • sssd-2-9
    • 838ee2f - tests: removing intg/test_confdb.py

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

Successfully merging this pull request may close these issues.

4 participants