Update get_targeted_hosts entity to work with new job inv wizard - #2487
Merged
LadislavVasina1 merged 1 commit intoJul 23, 2026
Conversation
Contributor
Author
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Relying on multiple fixed
time.sleep(3)calls will make this flow slow and potentially flaky; consider replacing them with explicit waits on the next button and the target hosts section being present/ready instead. - The method now conditionally advances the wizard by clicking
next_button, which changes its side effects; consider either renaming/splitting the method to make this behavior clear or centralizing the wizard navigation logic in a dedicated helper to keep responsibilities separated.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Relying on multiple fixed `time.sleep(3)` calls will make this flow slow and potentially flaky; consider replacing them with explicit waits on the next button and the target hosts section being present/ready instead.
- The method now conditionally advances the wizard by clicking `next_button`, which changes its side effects; consider either renaming/splitting the method to make this behavior clear or centralizing the wizard navigation logic in a dedicated helper to keep responsibilities separated.
## Individual Comments
### Comment 1
<location path="airgun/entities/job_invocation.py" line_range="70" />
<code_context>
time.sleep(3)
view = JobInvocationCreateView(self.browser)
time.sleep(3)
+ if view.next_button.is_displayed and view.next_button.is_enabled:
+ view.next_button.click()
</code_context>
<issue_to_address>
**suggestion (performance):** Consider replacing chained `time.sleep(3)` calls with an explicit wait on the relevant UI condition.
There are now two fixed 3-second sleeps around the `next_button` interaction, which both slow the test and remain brittle if the page timing changes. Please replace these with an explicit wait (e.g., waiting for `next_button` to be clickable or for the target hosts view to appear) to improve reliability and reduce runtime.
Suggested implementation:
```python
view = JobInvocationCreateView(self.browser)
# Explicitly wait until the "next" button is clickable instead of using fixed sleeps
WebDriverWait(self.browser, 10).until(
EC.element_to_be_clickable(view.next_button)
)
view.next_button.click()
self.browser.plugin.ensure_page_safe()
time.sleep(3)
return view.target_hosts_and_inputs.read()
```
To fully implement this change, you will also need to:
1. Add the appropriate imports at the top of `airgun/entities/job_invocation.py`, for example:
- `from selenium.webdriver.support.ui import WebDriverWait`
- `from selenium.webdriver.support import expected_conditions as EC`
2. If `self.browser` is not the raw Selenium WebDriver instance, adjust the `WebDriverWait` call to use the underlying driver (e.g., `self.browser.driver`), consistent with how other explicit waits are implemented in this codebase.
3. If the project already has helper methods for explicit waits (e.g., in `self.browser.plugin`), you may prefer to replace the `WebDriverWait` block with the project-standard helper for waiting until an element is clickable.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| @@ -70,6 +70,10 @@ def get_targeted_hosts(self): | |||
| time.sleep(3) | |||
Contributor
There was a problem hiding this comment.
suggestion (performance): Consider replacing chained time.sleep(3) calls with an explicit wait on the relevant UI condition.
There are now two fixed 3-second sleeps around the next_button interaction, which both slow the test and remain brittle if the page timing changes. Please replace these with an explicit wait (e.g., waiting for next_button to be clickable or for the target hosts view to appear) to improve reliability and reduce runtime.
Suggested implementation:
view = JobInvocationCreateView(self.browser)
# Explicitly wait until the "next" button is clickable instead of using fixed sleeps
WebDriverWait(self.browser, 10).until(
EC.element_to_be_clickable(view.next_button)
)
view.next_button.click()
self.browser.plugin.ensure_page_safe()
time.sleep(3)
return view.target_hosts_and_inputs.read()To fully implement this change, you will also need to:
- Add the appropriate imports at the top of
airgun/entities/job_invocation.py, for example:from selenium.webdriver.support.ui import WebDriverWaitfrom selenium.webdriver.support import expected_conditions as EC
- If
self.browseris not the raw Selenium WebDriver instance, adjust theWebDriverWaitcall to use the underlying driver (e.g.,self.browser.driver), consistent with how other explicit waits are implemented in this codebase. - If the project already has helper methods for explicit waits (e.g., in
self.browser.plugin), you may prefer to replace theWebDriverWaitblock with the project-standard helper for waiting until an element is clickable.
Contributor
|
PRT Result |
ColeHiggins2
approved these changes
Jul 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PF5 job invocation wizard landed and update is needed for it.
This PR is a direct fix for new failures of test_change_content_source