-
Notifications
You must be signed in to change notification settings - Fork 23.7k
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
Fix issues with ansible-playbook-callbacks test #82407
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,22 +96,22 @@ | |
ignore_errors: true | ||
|
||
- name: async poll ok | ||
command: sleep 2 | ||
async: 3 | ||
poll: 1 | ||
command: sleep 3 | ||
async: 5 | ||
poll: 2 | ||
|
||
- name: async poll failed | ||
shell: sleep 2; false | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same as above. |
||
async: 3 | ||
poll: 1 | ||
shell: sleep 3; false | ||
async: 5 | ||
poll: 2 | ||
ignore_errors: true | ||
|
||
- include_tasks: include_me.yml | ||
markgoddard marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
- name: diff | ||
copy: | ||
content: diff | ||
dest: '{{ remote_tmp_dir.path }}/diff.txt' | ||
dest: '{{ remote_tmp_dir }}/diff.txt' | ||
diff: true | ||
|
||
- hosts: i_dont_exist | ||
markgoddard marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,6 +5,7 @@ set -eux | |
export ANSIBLE_CALLBACK_PLUGINS=../support-callback_plugins/callback_plugins | ||
export ANSIBLE_ROLES_PATH=../ | ||
export ANSIBLE_STDOUT_CALLBACK=callback_debug | ||
export ANSIBLE_HOST_PATTERN_MISMATCH=warning | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I still think this should be set for the |
||
|
||
ansible-playbook all-callbacks.yml 2>/dev/null | sort | uniq -c | tee callbacks_list.out | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed as I have yet to see this being an issue in our CI. Generally we want the sleep to be as short as possible to reduce tests running time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I definitely hit the issue locally, and I saw it in CI once too, but that may have been with an earlier iteration of the values. Either way, it should be possible to run the integration tests on a laptop reliably.
I understand the concern around test runtime, and set it up to ensure that only one poll callback is sent per task.
The problem is that the sleep duration is a multiple of the poll interval, so the number of polls can vary. I chose these numbers to make the sleep finish in the middle of the 1st and 2nd poll.
I'll try halving all of the numbers, which should reduce the task duration to 2s, as it was before.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm testing this locally like this:
With the original numbers (sleep 2 poll 1) and it failed on the 2nd attempt. Tried again and it failed on the 5th attempt.
I tried it twice with the change proposed here (sleep 3 poll 2) and it did not fail.
I tried it with the numbers halved (sleep 1.5 poll 1) and it failed on the second attempt.
I tried a few other combinations, but couldn't find something that worked well (poll must be an integer).
So I'm inclined to keep the numbers proposed here - a slow test is better than an unreliable one.