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

ui_session: return airgun session as a context manager #15050

Merged

Conversation

pnovotny
Copy link
Contributor

@pnovotny pnovotny commented May 13, 2024

Problem Statement

If a UI test is using the new session approach via target_sat.ui_session(),
the browser window is not closed at the end of the test.
Also the screenshot is not taken if the test fails.

Solution

Satellite.ui_session() method needs to return the airgun Session as a context manager,
so the __exit__ method is called, which handles screenshot capture and closing the browser window.

In `Satellite.ui_session()` context manager method,
`airgun.session.Session()` is also a context manager
and needs to be returned using the `with-yield` contruct,
so the Session's `__enter__` and `__exit__` methods are executed.

This fixes the problem where `Session.__exit__()` method was not called
and thus screenshot was not taken and the browser window was not closed
at the end of the test.

Also the `except Exception: raise` part was removed as redundant.
@pnovotny pnovotny added CherryPick PR needs CherryPick to previous branches 6.12.z Introduced in or relating directly to Satellite 6.12 AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing 6.13.z Introduced in or relating directly to Satellite 6.13 6.14.z Introduced in or relating directly to Satellite 6.14 6.15.z Introduced in or relating directly to Satellite 6.15 labels May 13, 2024
@pnovotny pnovotny self-assigned this May 13, 2024
@pnovotny pnovotny requested review from pondrejk, lhellebr and a team May 13, 2024 16:40
@pnovotny
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_host.py -k test_positive_inherit_puppet_env_from_host_group_when_action

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6908
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/test_host.py -k test_positive_inherit_puppet_env_from_host_group_when_action --external-logging
Test Result : ========== 1 failed, 41 deselected, 58 warnings in 1689.24s (0:28:09) ==========

@Satellite-QE Satellite-QE added the PRT-Failed Indicates that latest PRT run is failed for the PR label May 13, 2024
@pnovotny
Copy link
Contributor Author

The test test_positive_inherit_puppet_env_from_host_group_when_action was meant to fail. Not by design, it needs to be fixed, but it serves the purpose for verification of this PR.

It is using target_sat.ui_session() and from the test run log (PRT job no.6908 - logs/robottelo.log), the screenshot is now taken:

2024-05-13 17:32:26 - airgun.session - INFO - Stopping UI session 'test_positive_inherit_puppet_env_from_host_group_when_action' for user 'admin'
2024-05-13 17:32:26 - airgun.session - DEBUG - Saving screenshot /opt/app-root/src/robottelo/screenshots/2024-05-13/test_positive_inherit_puppet_env_from_host_group_when_action-screenshot-2024-05-13_17_32_26.png

Also the test session video recording shows it takes 40s (session 24fe1fd0680d15575fcf6ecce53e71e2).
In comparison with the regular test run w/o this PR, for example job sat-stream-rhel8-Hosts no.60, the video takes 05m45s (session 890fcb6b17c2e7b8dc9e0c151b356ac9),
which confirms that the browser was still running for another 5 minutes, which is the Selenium Grid timeout after which it was killed.

@pnovotny pnovotny added PRT-Passed Indicates that latest PRT run is passed for the PR and removed PRT-Failed Indicates that latest PRT run is failed for the PR labels May 14, 2024
@pnovotny pnovotny marked this pull request as ready for review May 14, 2024 13:07
@pnovotny pnovotny requested a review from a team as a code owner May 14, 2024 13:07
Copy link
Member

@rplevka rplevka left a comment

Choose a reason for hiding this comment

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

Great catch! This seems to be the solution to those hanging sessions (before they finally time out).
It's a really important patch if true.
LGTM

@omkarkhatavkar
Copy link

trigger: test-robottelo
pytest: tests/foreman/ui/ -m e2e

Copy link

@omkarkhatavkar omkarkhatavkar left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this, @pnovotny! The patch works nicely; I've checked it in some cases. I'm running some more PRT tests just to ensure everything is working as expected. Thanks again, @pnovotny!

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 6934
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/ -m e2e --external-logging
Test Result : = 18 failed, 41 passed, 1 skipped, 648 deselected, 3564 warnings, 36 errors in 24051.25s (6:40:51) =

@Satellite-QE Satellite-QE added PRT-Failed Indicates that latest PRT run is failed for the PR and removed PRT-Passed Indicates that latest PRT run is passed for the PR labels May 15, 2024
@omkarkhatavkar
Copy link

@pnovotny I saw some failures on the PRT job related to InvalidSessionIdException. After doing some more debugging come to know these are because of extra with statement we are doing in the tests. After this fix do not need the extra with context manager in tests.

failed on teardown with "selenium.common.exceptions.InvalidSessionIdException: Message: Unable to find session with ID: 5534727cf28df655b9afb89ae30a54d5
Build info: version: '4.17.0', revision: 'e52b1be057*'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '4.18.0-513.9.1.el8_9.x86_64', java.version: '11.0.21'
Driver info: driver.version: unknown
Stacktrace:
org.openqa.selenium.NoSuchSessionException: Unable to find session with ID: 5534727cf28df655b9afb89ae30a54d5
Build info: version: '4.17.0', revision: 'e52b1be057*'
System info: os.name: 'Linux', os.arch: 'amd64', os.version: '4.18.0-513.9.1.el8_9.x86_64', java.version: '11.0.21'
Driver info: driver.version: unknown

@omkarkhatavkar
Copy link

@pnovotny did you get chance to look into those failures ?

@pnovotny
Copy link
Contributor Author

@pnovotny did you get chance to look into those failures ?

Yes, thanks @omkarkhatavkar for raising the issue with the session ID exceptions.
From my investigation, the culprit can be that now the session fixture returns a nested context managers (two to be exact),
which then calls the browser teardown twice and the second attempt fails, because the browser is already closed.
I hope SatelliteQE/airgun#1390 will fix it.
Let me run few PRTs to see if it helped...

@pnovotny
Copy link
Contributor Author

trigger: test-robottelo
pytest: tests/foreman/ui/test_activationkey.py -k test_positive_access_non_admin_user -m e2e
airgun: 1390

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7052
Build Status: SUCCESS
PRT Comment: pytest tests/foreman/ui/test_activationkey.py -k test_positive_access_non_admin_user -m e2e --external-logging
Test Result : ========== 1 passed, 61 deselected, 76 warnings in 839.74s (0:13:59) ===========

@Satellite-QE Satellite-QE added PRT-Passed Indicates that latest PRT run is passed for the PR and removed PRT-Failed Indicates that latest PRT run is failed for the PR labels May 21, 2024
@omkarkhatavkar
Copy link

trigger: test-robottelo
pytest: tests/foreman/ui/ -m e2e

@omkarkhatavkar
Copy link

trigger: test-robottelo
pytest: tests/foreman/ui/ -m e2e
airgun: 1390

@Satellite-QE
Copy link
Collaborator

PRT Result

Build Number: 7055
Build Status: UNSTABLE
PRT Comment: pytest tests/foreman/ui/ -m e2e --external-logging
Test Result : = 12 failed, 47 passed, 1 skipped, 652 deselected, 3663 warnings, 4 errors in 28407.49s (7:53:27) =

@Satellite-QE Satellite-QE added PRT-Failed Indicates that latest PRT run is failed for the PR and removed PRT-Passed Indicates that latest PRT run is passed for the PR labels May 22, 2024
@omkarkhatavkar
Copy link

@pnovotny I reviewed the failures and they do not appear to be specifically related to this PR. Overall, everything looks good to me.

@omkarkhatavkar omkarkhatavkar merged commit 2618e01 into SatelliteQE:master May 23, 2024
23 of 24 checks passed
github-actions bot pushed a commit that referenced this pull request May 23, 2024
In `Satellite.ui_session()` context manager method,
`airgun.session.Session()` is also a context manager
and needs to be returned using the `with-yield` contruct,
so the Session's `__enter__` and `__exit__` methods are executed.

This fixes the problem where `Session.__exit__()` method was not called
and thus screenshot was not taken and the browser window was not closed
at the end of the test.

Also the `except Exception: raise` part was removed as redundant.

(cherry picked from commit 2618e01)
github-actions bot pushed a commit that referenced this pull request May 23, 2024
In `Satellite.ui_session()` context manager method,
`airgun.session.Session()` is also a context manager
and needs to be returned using the `with-yield` contruct,
so the Session's `__enter__` and `__exit__` methods are executed.

This fixes the problem where `Session.__exit__()` method was not called
and thus screenshot was not taken and the browser window was not closed
at the end of the test.

Also the `except Exception: raise` part was removed as redundant.

(cherry picked from commit 2618e01)
github-actions bot pushed a commit that referenced this pull request May 23, 2024
In `Satellite.ui_session()` context manager method,
`airgun.session.Session()` is also a context manager
and needs to be returned using the `with-yield` contruct,
so the Session's `__enter__` and `__exit__` methods are executed.

This fixes the problem where `Session.__exit__()` method was not called
and thus screenshot was not taken and the browser window was not closed
at the end of the test.

Also the `except Exception: raise` part was removed as redundant.

(cherry picked from commit 2618e01)
github-actions bot pushed a commit that referenced this pull request May 23, 2024
In `Satellite.ui_session()` context manager method,
`airgun.session.Session()` is also a context manager
and needs to be returned using the `with-yield` contruct,
so the Session's `__enter__` and `__exit__` methods are executed.

This fixes the problem where `Session.__exit__()` method was not called
and thus screenshot was not taken and the browser window was not closed
at the end of the test.

Also the `except Exception: raise` part was removed as redundant.

(cherry picked from commit 2618e01)
@pnovotny
Copy link
Contributor Author

@omkarkhatavkar thank you for checking the test results.
I went through the PRT failures in the 6.15, 6.14, 6.13 and 6.12 cherry-picks and I also haven't found any problems related to this PR.

Gauravtalreja1 pushed a commit that referenced this pull request May 29, 2024
ui_session: return airgun session as a context manager (#15050)

In `Satellite.ui_session()` context manager method,
`airgun.session.Session()` is also a context manager
and needs to be returned using the `with-yield` contruct,
so the Session's `__enter__` and `__exit__` methods are executed.

This fixes the problem where `Session.__exit__()` method was not called
and thus screenshot was not taken and the browser window was not closed
at the end of the test.

Also the `except Exception: raise` part was removed as redundant.

(cherry picked from commit 2618e01)

Co-authored-by: Pavel Novotny <[email protected]>
Gauravtalreja1 pushed a commit that referenced this pull request May 29, 2024
ui_session: return airgun session as a context manager (#15050)

In `Satellite.ui_session()` context manager method,
`airgun.session.Session()` is also a context manager
and needs to be returned using the `with-yield` contruct,
so the Session's `__enter__` and `__exit__` methods are executed.

This fixes the problem where `Session.__exit__()` method was not called
and thus screenshot was not taken and the browser window was not closed
at the end of the test.

Also the `except Exception: raise` part was removed as redundant.

(cherry picked from commit 2618e01)

Co-authored-by: Pavel Novotny <[email protected]>
Gauravtalreja1 pushed a commit that referenced this pull request May 29, 2024
ui_session: return airgun session as a context manager (#15050)

In `Satellite.ui_session()` context manager method,
`airgun.session.Session()` is also a context manager
and needs to be returned using the `with-yield` contruct,
so the Session's `__enter__` and `__exit__` methods are executed.

This fixes the problem where `Session.__exit__()` method was not called
and thus screenshot was not taken and the browser window was not closed
at the end of the test.

Also the `except Exception: raise` part was removed as redundant.

(cherry picked from commit 2618e01)

Co-authored-by: Pavel Novotny <[email protected]>
Gauravtalreja1 pushed a commit that referenced this pull request May 29, 2024
ui_session: return airgun session as a context manager (#15050)

In `Satellite.ui_session()` context manager method,
`airgun.session.Session()` is also a context manager
and needs to be returned using the `with-yield` contruct,
so the Session's `__enter__` and `__exit__` methods are executed.

This fixes the problem where `Session.__exit__()` method was not called
and thus screenshot was not taken and the browser window was not closed
at the end of the test.

Also the `except Exception: raise` part was removed as redundant.

(cherry picked from commit 2618e01)

Co-authored-by: Pavel Novotny <[email protected]>
jyejare pushed a commit to jyejare/robottelo that referenced this pull request Oct 19, 2024
…5050)

In `Satellite.ui_session()` context manager method,
`airgun.session.Session()` is also a context manager
and needs to be returned using the `with-yield` contruct,
so the Session's `__enter__` and `__exit__` methods are executed.

This fixes the problem where `Session.__exit__()` method was not called
and thus screenshot was not taken and the browser window was not closed
at the end of the test.

Also the `except Exception: raise` part was removed as redundant.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.12.z Introduced in or relating directly to Satellite 6.12 6.13.z Introduced in or relating directly to Satellite 6.13 6.14.z Introduced in or relating directly to Satellite 6.14 6.15.z Introduced in or relating directly to Satellite 6.15 AutoMerge_Cherry_Picked The cherrypicked PRs of master PR would be automerged if all checks passing CherryPick PR needs CherryPick to previous branches PRT-Failed Indicates that latest PRT run is failed for the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants