Issue#743: fetch.ubuntu._run_with_retries raises on unexpected exitcode #744
Conversation
This file contains 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
|
@kwmonroe do you like green tests? Separate PR or can they be merged together |
addyess
commented
Oct 26, 2022
Comment on lines
+307
to
+310
| request_time = '2021-02-02 10:19:55' | ||
| timestamp = datetime.datetime.strptime( | ||
| 'Tue ' + request_time + ' UTC', | ||
| '%a %Y-%m-%d %H:%M:%S %Z').timestamp() |
There was a problem hiding this comment.
the timestamp is seconds since epoch -- but it's timezone specific. This change was so this test would run successfully on all machines regardless of the locale's timezone. (ie -- it failed on my machine but was okay in other places where there was no TZ set)
addyess
commented
Oct 26, 2022
Comment on lines
+49
to
+51
| @mock.patch.object(hookenv, '_run_atstart') | ||
| @mock.patch.object(hookenv, 'config', mock.Mock()) | ||
| def test_manage_stop(self, _run_atstart, hook_name, stop_services, reconfigure_services): |
There was a problem hiding this comment.
adjust most of the test_manage_* tests such that they don't ACTUALLY run _run_atstart -- but use a mock instead
addyess
commented
Oct 26, 2022
| assert not stop_services.called | ||
| reconfigure_services.assert_called_once_with() | ||
| provide_data.assert_called_once_with() | ||
|
|
||
| @mock.patch.object(hookenv, '_atstart', []) |
There was a problem hiding this comment.
except for the test_manage_calls_atstart which guarantees hookenv._atstart is empty before the test runs
kwmonroe
reviewed
Oct 27, 2022
There was a problem hiding this comment.
This looks nice, and I appreciate the focus on green tests for py310. I'm confused how this 3.10 failure didn't show up in py <= 3.8:
ERROR: test_manage_calls_atexit (tests.core.test_services.TestServiceManager)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/runner/work/charm-helpers/charm-helpers/charmhelpers/core/hookenv.py", line 1180, in inner_translate_exc2
return f(*args, **kwargs)
File "/home/runner/work/charm-helpers/charm-helpers/charmhelpers/core/hookenv.py", line 1218, in is_leader
return json.loads(subprocess.check_output(cmd).decode('UTF-8'))
File "/opt/hostedtoolcache/Python/3.10.8/x64/lib/python3.10/subprocess.py", line [42](https://github.com/juju/charm-helpers/actions/runs/3331110013/jobs/5510456204#step:7:43)1, in check_output
return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
File "/opt/hostedtoolcache/Python/3.10.8/x64/lib/python3.10/subprocess.py", line 503, in run
with Popen(*popenargs, **kwargs) as process:
File "/opt/hostedtoolcache/Python/3.10.8/x64/lib/python3.10/subprocess.py", line 971, in __init__
self._execute_child(args, executable, preexec_fn, close_fds,
File "/opt/hostedtoolcache/Python/3.10.8/x64/lib/python3.10/subprocess.py", line 1847, in _execute_child
raise child_exception_type(errno_num, err_msg, err_filename)
FileNotFoundError: [Errno 2] No such file or directory: 'is-leader'
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
File "/home/runner/work/charm-helpers/charm-helpers/tests/core/test_services.py", line 81, in test_manage_calls_atexit
manager.manage()
File "/home/runner/work/charm-helpers/charm-helpers/charmhelpers/core/services/base.py", line 130, in manage
hookenv._run_atstart()
File "/home/runner/work/charm-helpers/charm-helpers/charmhelpers/core/hookenv.py", line 13[48](https://github.com/juju/charm-helpers/actions/runs/3331110013/jobs/5510456204#step:7:49), in _run_atstart
callback(*args, **kwargs)
File "/home/runner/work/charm-helpers/charm-helpers/charmhelpers/coordinator.py", line 3[59](https://github.com/juju/charm-helpers/actions/runs/3331110013/jobs/5510456204#step:7:60), in handle
if not hookenv.is_leader():
File "/home/runner/work/charm-helpers/charm-helpers/charmhelpers/core/hookenv.py", line 1182, in inner_translate_exc2
raise to_exc
NotImplementedError
Perhaps a nose vs nose2 change. Perhaps gremlins in the 3.8 vs 3.10 basepython. At any rate, i'm comfortable with the current impl and think passing 3.10 outweighs why <= 3.8 weren't always failing.
brianphaley
pushed a commit
to brianphaley/charm-helpers
that referenced
this pull request
Apr 5, 2024
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.
this method gives a false success when in reality it should fail immediately (no retrying)
the method should raise when the exit value of
subprocess.check_callis an error not on the rety listTLDR:
if its exit value is 0 -- 🎉
if the exit value is in the
retry_results-- retry max_retries like expectedif the exit value is not in the
retry_results-- 🔥Addresses #743