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/ts_cache.py #7580

Closed
wants to merge 2 commits into from

Conversation

danlavu
Copy link

@danlavu danlavu commented Sep 9, 2024

the following test cases are now covered in system/test_cache.py and this can be removed.

  • fixed assertion writes_to_both_databases tests
  • added test detecting modification and deletion for groups
    • test is a common user story and functional, changed priority to critical
  • added "integration" test invalidating user, group, netgroup objects

@shridhargadekar
Copy link
Contributor

The following test cases are covered in system/test_cache.py

Could you share where exactly these tests getting covered in system/test_cache.py?
Some of old test cases have set up related to rfc2307bis, where these scenarios are being covered

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 where exactly these rfc2307bis related tests getting covered in system/test_cache.py?

@alexey-tikhonov
Copy link
Member

CI is red

@danlavu
Copy link
Author

danlavu commented Sep 12, 2024

@shridhargadekar It's not. We're not porting tests, the coverage is related to cache timestamp in ldb which is covered.

I asked the team how to determine what LDAP tests we should test against rfc2307 and rfc2307bis, but we can easily take any test and parameterize the test to re-run it against 2307bis, so this can easily be done later.

@danlavu danlavu force-pushed the tests-rm-intg-ts_cache branch 2 times, most recently from a6649d3 to 9e764d8 Compare September 12, 2024 15:58
@shridhargadekar
Copy link
Contributor

shridhargadekar commented Sep 13, 2024

@danlavu not all tests have the coverage, specifically L655 L628
These scenarios do not have coverage.
The way these tests running is bit different from what tests/test_cache.py

@shridhargadekar
Copy link
Contributor

If missing coverage is done later then consider approval from me.

@danlavu
Copy link
Author

danlavu commented Sep 17, 2024

@shridhargadekar
I admit, I didn't take a super close look at the test cases. I'm going to go through them today and will probably end up writing tests for them.

@danlavu
Copy link
Author

danlavu commented Sep 18, 2024

I added two tests that cover the missing scenarios you mentioned.

Test that ensures the group changes are reflected after a clear
Another test, that ensures the cache records the change when that object type is cleared.

Updated the assertion for one of the tests that was confusing, mentioned in slack.

@danlavu
Copy link
Author

danlavu commented Sep 18, 2024

Tests are not green, looking into it.

@danlavu
Copy link
Author

danlavu commented Sep 19, 2024

The test was not working correctly, it was passing when there were no results, fixed now.

@danlavu
Copy link
Author

danlavu commented Oct 21, 2024

@alexey-tikhonov, I converted all the tests to use the LDAP provider, as we discussed, and I corrected the logic; it's working now and not being skipped. I did notice that netgroups in the timestamp cache is empty. There is nothing in the results. Do you know if netgroups is supposed to write to the timestamp cache?

@alexey-tikhonov
Copy link
Member

@shridhargadekar, please review the latest version.

Dan Lavu added 2 commits November 4, 2024 19:00
the following test cases are now covered in system/test_cache.py and
this can be removed.

* fixed assertion writes_to_both_databases tests
* added test detecting modification and deletion for groups
** test is a common user story and functional, changed priority to
critical
* added "integration" test invalidating user, group, netgroup objects
There is minimal benefit to run these tests against all providers.
@danlavu
Copy link
Author

danlavu commented Nov 6, 2024

The failure is not related to the patch.

@alexey-tikhonov
Copy link
Member

@danlavu, there is a conflict in sssd-2-9, please open explicit backport PR if needed.

@alexey-tikhonov
Copy link
Member

Pushed PR: #7580

  • master
    • d5b6484 - tests: converting all the ldb cache tests to use one provider
    • be0c232 - tests: removing intg/ts_cache.py
  • sssd-2-10
    • 0ceefae - tests: converting all the ldb cache tests to use one provider
    • c6b9e26 - tests: removing intg/ts_cache.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.

3 participants