Provide better error output for test_satellite_installation #17105

ekohl · 2024-12-06T14:01:26Z

Problem Statement

The current output of test_satellite_installation is useless. Because the output is so long, pytest truncates it which hides the relevant error.

Solution

As a first step, it does a trivial cleanup for readability. The next commit stops using len(x) == 0 and instead uses not x to assert there is no output. I haven't tested this, but I believe this allows pytest to skip a layer of output and hopefully also provide better output.

The real change is that it asserts the exit code of the installer to be 0 or 2. In practice I'd always expect 2, but 0 is also valid. See https://github.com/theforeman/kafo?tab=readme-ov-file#exit-code for more information. Since foreman-installer 3.8.0 (theforeman/foreman-installer@0d2fb67) it also displays structured output on the exact failure.

Related Issues

trigger: test-robottelo
pytest: tests/foreman/ui/test_contenthost.py -k 'test_satellite_installation'

Quoting PEP 20: > Flat is better than nested.

This should help pytest to provide a better output.

If the installer failed, there's no point to continue. Since Foreman 3.8 it provides exactly which steps failed which is preferred over "parsing" the log file. Having an assertion in a fixture should result in the the test showing up as ERROR. Link: https://medium.com/opsops/pytest-fail-in-a-fixture-and-in-a-test-2122fab0564 Link: theforeman/foreman-installer@0d2fb67

ekohl

I had some issues with the installer_satellite fixture. I don't like that it always asserts success. Ideally speaking we would also test for failures. When I looked I could only find a single test that used this fixture, so perhaps it should be refactored to just present an empty machine and have the installer command be part of the test itself.

ekohl · 2024-12-06T14:02:59Z

pytest_fixtures/core/sat_cap_factory.py

@@ -395,6 +394,9 @@ def installer_satellite(request):
        ).get_command(),
        timeout='30m',
    )
+    # exit code 0 means no changes, 2 means changes were applied succesfully
+    assert installer_result.status in (0, 2), installer_result.stdout


I also debated assert 'There were errors detected during install not in installer_result.stdout` or similar.

Given we have the failed_when check based on satellite-installer RC, I wouldn't diverge from that in robottelo.

https://github.com/theforeman/foreman-operations-collection/blob/2bf0408ab1469908ac490878a91da388a4bbdba4/roles/installer/tasks/main.yml#L41

ogajduse

Nice and clean, thank you!

When I looked I could only find a single test that used this fixture, so perhaps it should be refactored to just present an empty machine and have the installer command be part of the test itself.

From my point of view, this seems like a good idea. Would you be open to making the change sometime in the future?

ogajduse · 2024-12-09T12:24:49Z

pytest_fixtures/core/sat_cap_factory.py

@@ -395,6 +394,9 @@ def installer_satellite(request):
        ).get_command(),
        timeout='30m',
    )
+    # exit code 0 means no changes, 2 means changes were applied succesfully
+    assert installer_result.status in (0, 2), installer_result.stdout


Given we have the failed_when check based on satellite-installer RC, I wouldn't diverge from that in robottelo.

https://github.com/theforeman/foreman-operations-collection/blob/2bf0408ab1469908ac490878a91da388a4bbdba4/roles/installer/tasks/main.yml#L41

rmynar

Looks good. I have just added some labels to cherrypick this into older branches.

The installer_satellite fixture was meant to be used within ipv6 installation test. But we decided to use different approach to test ipv4 and ipv6 installations. And the ipv6 stubbed testcases were removed recently - #16963

ekohl · 2024-12-09T15:03:25Z

Would you be open to making the change sometime in the future?

I'm open to it, but I can't promise anything.

The installer_satellite fixture was meant to be used within ipv6 installation test. But we decided to use different approach to test ipv4 and ipv6 installations. And the ipv6 stubbed testcases were removed recently - #16963

Does that mean we can remove a lot of the fixture? Or really, I see there's also sat_ready_rhel and cap_ready_rhel. Is there a reason we can't use those for the test?

My confusion is mostly from various tests like test_positive_install_sat_with_katello_certs that largely duplicate the whole setup steps, but I don't feel comfortable restructuring the fixture organization.

jameerpathan111 · 2024-12-10T09:40:35Z

trigger: test-robottelo
pytest: tests/foreman/installer/test_installer.py -k test_satellite_installation

Satellite-QE · 2024-12-10T10:11:45Z

PRT Result

Build Number: 9571
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/installer/test_installer.py -k test_satellite_installation --external-logging
Test Result : ========== 1 passed, 19 deselected, 32 warnings in 1493.58s (0:24:53) ==========

jameerpathan111 · 2024-12-10T10:43:01Z

Does that mean we can remove a lot of the fixture? Or really, I see there's also sat_ready_rhel and cap_ready_rhel. Is there a reason we can't use those for the test?

IMO test code has been heavily changed and has become more complicated because of build_sanity. installer_satellite fixture was created for that purpose. It'd be quite a challenge to refactor it without touching sanity part. I'd rather leave the task of simplifying code to JPL team or at least make them involve. For now, let's merge this PR and we can discuss further refactoring tasks with JPL.

(cherry picked from commit c170b84)

ekohl · 2024-12-10T12:18:44Z

I'd rather leave the task of simplifying code to JPL team or at least make them involve. For now, let's merge this PR and we can discuss further refactoring tasks with JPL.

That makes sense to me. When I was thinking about it, I think the fixture does too many things but untangling requires much more thought.

ogajduse · 2024-12-11T12:15:00Z

@ekohl Can you please backport your patch to the 6.14.z branch? ref. #17138

ekohl · 2024-12-13T13:46:28Z

#17172

ekohl added 3 commits December 6, 2024 14:37

Use elif for better readability

7b6bc5b

Quoting PEP 20: > Flat is better than nested.

Use assert not x instead of assert len(x) == 0

d3b6909

This should help pytest to provide a better output.

ekohl requested review from a team as code owners December 6, 2024 14:01

github-actions bot assigned ekohl Dec 6, 2024

ekohl commented Dec 6, 2024

View reviewed changes

ogajduse approved these changes Dec 9, 2024

View reviewed changes

rmynar approved these changes Dec 9, 2024

View reviewed changes

jameerpathan111 added the Stream Introduced in or relating directly to Satellite Stream/Master label Dec 10, 2024

Satellite-QE added the PRT-Passed Indicates that latest PRT run is passed for the PR label Dec 10, 2024

jameerpathan111 approved these changes Dec 10, 2024

View reviewed changes

lpramuk approved these changes Dec 10, 2024

View reviewed changes

ogajduse merged commit c170b84 into SatelliteQE:master Dec 10, 2024
14 of 15 checks passed

github-actions bot pushed a commit that referenced this pull request Dec 10, 2024

Provide better error output for test_satellite_installation (#17105)

d76ed29

(cherry picked from commit c170b84)

github-actions bot pushed a commit that referenced this pull request Dec 10, 2024

Provide better error output for test_satellite_installation (#17105)

1b9310f

(cherry picked from commit c170b84)

ekohl deleted the better-installer-sanity branch December 10, 2024 12:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide better error output for test_satellite_installation #17105

Provide better error output for test_satellite_installation #17105

ekohl commented Dec 6, 2024

ekohl left a comment

ekohl Dec 6, 2024

ogajduse Dec 9, 2024

ogajduse left a comment

ogajduse Dec 9, 2024

rmynar left a comment

ekohl commented Dec 9, 2024

jameerpathan111 commented Dec 10, 2024

Satellite-QE commented Dec 10, 2024

jameerpathan111 commented Dec 10, 2024

ekohl commented Dec 10, 2024

ogajduse commented Dec 11, 2024

ekohl commented Dec 13, 2024

Provide better error output for test_satellite_installation #17105

Provide better error output for test_satellite_installation #17105

Conversation

ekohl commented Dec 6, 2024

Problem Statement

Solution

Related Issues

ekohl left a comment

Choose a reason for hiding this comment

ekohl Dec 6, 2024

Choose a reason for hiding this comment

ogajduse Dec 9, 2024

Choose a reason for hiding this comment

ogajduse left a comment

Choose a reason for hiding this comment

ogajduse Dec 9, 2024

Choose a reason for hiding this comment

rmynar left a comment

Choose a reason for hiding this comment

ekohl commented Dec 9, 2024

jameerpathan111 commented Dec 10, 2024

Satellite-QE commented Dec 10, 2024

jameerpathan111 commented Dec 10, 2024

ekohl commented Dec 10, 2024

ogajduse commented Dec 11, 2024

ekohl commented Dec 13, 2024