Skip to content

Tests: Skip tests unstable on other architectures. - #8138

Closed
jakub-vavra-cz wants to merge 1 commit into
SSSD:sssd-2-9from
jakub-vavra-cz:multiarch-skip-2-9
Closed

jakub-vavra-cz wants to merge 1 commit into
SSSD:sssd-2-9from
jakub-vavra-cz:multiarch-skip-2-9

Conversation

@jakub-vavra-cz

Copy link
Copy Markdown
Contributor

Cherry pick of the #8135 on sssd-2-9

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request skips several tests that are unstable on architectures other than x86_64. The approach is sound, but the implementation introduces significant code duplication across multiple test files. My review focuses on refactoring this duplicated logic into a reusable helper or fixture to improve code maintainability, which is a high-priority concern. By centralizing the architecture check, future modifications will be much simpler and less error-prone.

Comment on lines +34 to +37
arch = multihost.client[0].run_command(
'uname -m', raiseonerr=False).stdout_text
if 'x86_64' not in arch:
pytest.skip("Test is not stable on other architectures.")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

This architecture check logic is duplicated in test_adtrust.py and test_misc.py. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, this logic should be extracted into a shared helper function or a pytest fixture.

A pytest fixture placed in a common conftest.py (e.g., src/tests/multihost/conftest.py) would be an idiomatic solution. For example:

# in a common conftest.py
import pytest

@pytest.fixture
def skip_unstable_on_non_x86_64(multihost):
    """Skips tests on non-x86_64 architectures."""
    arch = multihost.client[0].run_command(
        'uname -m', raiseonerr=False).stdout_text
    if 'x86_64' not in arch:
        pytest.skip("Test is not stable on other architectures.")

You could then apply this fixture to the test methods that require this check using @pytest.mark.usefixtures("skip_unstable_on_non_x86_64"). This would make the tests cleaner and the logic centralized.

Comment on lines +70 to +73
arch = multihost.client[0].run_command(
'uname -m', raiseonerr=False).stdout_text
if 'x86_64' not in arch:
pytest.skip("Test is not stable on other architectures.")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

This is the same duplicated architecture check logic as in test_kcm.py. This should be replaced by a call to a shared helper function or by using a common pytest fixture as suggested in the other comment to avoid code duplication.

Comment on lines +605 to +608
arch = multihost.client[0].run_command(
'uname -m', raiseonerr=False).stdout_text
if 'x86_64' not in arch:
pytest.skip("Test is not stable on other architectures.")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

This is another instance of the duplicated architecture check. Please consolidate this logic into a single shared helper or fixture to improve maintainability.

Comment on lines +218 to +221
arch = multihost.client[0].run_command(
'uname -m', raiseonerr=False).stdout_text
if 'x86_64' not in arch:
pytest.skip("Test is not stable on other architectures.")

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

This is another instance of the duplicated architecture check. This should be refactored into a shared utility to avoid repetition and make future changes easier.

@shridhargadekar

Copy link
Copy Markdown
Contributor

@jakub-vavra-cz could you check why are tests failing?

@jakub-vavra-cz

Copy link
Copy Markdown
Contributor Author

@jakub-vavra-cz could you check why are tests failing?

test_kcm - timeout issue beaker <-> openstack
ipa/test_asymmetric_auth_for_nsupdate - hostname/dns issues
ipa/trust - connectivity issues with master.testrealm.test resolution failed, offline failures.

@shridhargadekar shridhargadekar left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

lgtm

@jakub-vavra-cz

Copy link
Copy Markdown
Contributor Author

Pushed PR: #8138

  • sssd-2-9
    • 0122693 - Tests: Skip tests unstable on other architectures.
  • sssd-2-9-4
    • 04bd09b - Tests: Skip tests unstable on other architectures.

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.

2 participants