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: remove multihost basic tests #7493

Closed
wants to merge 1 commit into from

Conversation

danlavu
Copy link

@danlavu danlavu commented Jul 12, 2024

  • test_ifp.py
    • test has overlapping coverage from system/test_infopipe.py
  • test_kcm.py
    • test has overlapping coverage from system/test_kdm.py and authselect/system/test_sssd.py , the functional credential delegation
    • a functional test has been added to the test plan for kcm
  • test_ldapapi.py
    • tests are low priority with a larger effort to move.
    • test configures ldap, using the 389 slapd file for it's URI, this test can only be performed on a server and does not offer much value. this test has been added to the test plan and will be re-implemented if approved.

Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

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

I believe you need to also remove the test from the github ci workflow as well. That's currently showing:

ERROR: file or directory not found: ./sssd/src/tests/multihost/basic

Copy link
Contributor

@spoore1 spoore1 left a comment

Choose a reason for hiding this comment

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

I believe that's good. We'll see when the CI tests complete but, I think it 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.

Great, ack! Before we push, did you already remove these tests from IdM CI?

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.

You also have to remove multihost from "result" job dependencies:

result:
name: All tests are successful
if: ${{ always() }}
runs-on: ubuntu-latest
needs: [build, intgcheck, multihost, system]
steps:
- name: Fail on failure
if: |
needs.build.result != 'success'
|| needs.intgcheck.result != 'success'
|| needs.multihost.result != 'success'
|| needs.system.result != 'success'
run: exit 1

@danlavu danlavu requested a review from pbrezina July 17, 2024 10:51
@danlavu
Copy link
Author

danlavu commented Jul 17, 2024

Great, ack! Before we push, did you already remove these tests from IdM CI?

I just looked and multihost/basic directory is not used in the test plan any where. I don't think they were ever in there.

@jakub-vavra-cz
Copy link
Contributor

We need to change all legacy distgit branches that refers to this as osci.tier0 before doing this othewise we break stuff especially for sustaining engineering.

@jakub-vavra-cz
Copy link
Contributor

We need to change all legacy distgit branches that refers to this as osci.tier0 before doing this othewise we break stuff especially for sustaining engineering.

I did go through sssd dist git branches and removed the requirement for osci.tier0. Hopefully we are good to go now.

* test_ifp.py test are now convered in system/test_infopipe.py
* test_kcm.py test are now covered in system/test_kdm.py and
  authselect/system/test_sssd.py , the functional credential delegation
** a functional test has been added to the test plan
* test_ldapapi.py tests are low priority with a larger effort to move.
** test configures ldap, using the 389 slapd file for it's URI, this
test can only be performed on a server and does not offer much value.
this test has been added to the test plan and will be re-implemented if
approved.
@danlavu
Copy link
Author

danlavu commented Jul 29, 2024

rebased, should be good to push as long as the tests are successful

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.

Please, also update https://sssd.io/contrib/tests/multihost-tests.html and when on it, perhaps mention current status and link test.sssd.io in https://sssd.io/contrib/running-tests.html

@danlavu
Copy link
Author

danlavu commented Jul 30, 2024

Please, also update https://sssd.io/contrib/tests/multihost-tests.html and when on it, perhaps mention current status and link test.sssd.io in https://sssd.io/contrib/running-tests.html

ACK, added it to my TODO.

@pbrezina
Copy link
Member

pbrezina commented Aug 2, 2024

Pushed PR: #7493

  • master
    • c3ce4bc - tests: remove multihost basic tests

@pbrezina pbrezina added Pushed and removed Accepted Ready to push Ready to push labels Aug 2, 2024
@pbrezina pbrezina closed this Aug 2, 2024
@pbrezina
Copy link
Member

pbrezina commented Aug 2, 2024

There is conflict in sssd-2-9 and sssd-2-8.

@danlavu danlavu deleted the tests-alltests branch November 4, 2024 17:24
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.

5 participants