-
Notifications
You must be signed in to change notification settings - Fork 0
[QA-269] retryFetch Fails #1
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
Conversation
20d710d
to
df5f9bf
Compare
ExpectListdir(dir='source') | ||
.files(['.git']) | ||
.exit(0), | ||
ExpectShell(workdir='source', |
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 may have misunderstood how these tests are working, but why are we expecting an exit code of 1 for this?
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.
It's not expecting the exit code.
ExpectShell
configures a mock which expects the git fetch
command and then imitates a non-zero exit code.
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.
Ah ok, that's handy!
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.
The change makes sense to me and the tests are fair enough. 👍
Note that the `test_mode_full_copy_recursive_fetch_fail_retry_{fail,succeed}` test both fail to execute an expected second attempt to git fetch, and the `test_mode_full_copy_recursive_fetch_fail_clobberOnFailure` test fails to clobber and clone the source directory: ``` $ trial buildbot.test.unit.steps.test_source_git.TestGit ... (etc) ... test_mode_full_copy_recursive ... [OK] test_mode_full_copy_recursive_fetch_fail ... [OK] test_mode_full_copy_recursive_fetch_fail_clobberOnFailure ... [ERROR] test_mode_full_copy_recursive_fetch_fail_retry_fail ... [ERROR] test_mode_full_copy_recursive_fetch_fail_retry_succeed ... [ERROR] ```
df5f9bf
to
831c2b9
Compare
…requested. In `Git._fetchOrFallback`, the call to `self._fetch(None)` will throw a `BuildStepFailed` if the fetch command fails. The fallback code in `_fetchOrFallback` will not be executed and the fetch will not be retried nor will `clobber()` be called. This change modifies `_fetch` to accept an `abandonOnFailure` flag to control whether it should throw. Test Plan Run the unit tests added in the previous commit.
831c2b9
to
7642e8c
Compare
Fix
retryFetch
andclobberOnFailure
.As per http://docs.buildbot.net/latest/manual/configuration/steps/source_git.html#
However, this does not currently work.
In
Git._fetchOrFallback
, the call toself._fetch(None)
will throw aBuildStepFailed
if the fetch command fails. The fallback code in_fetchOrFallback
will not be executed and the fetch will not be retried. Same forclobberOnFailure=True
if it is requested.This change modifies
_fetch
to accept anabandonOnFailure
flag to control whether it should throw.Test Plan
In the first commit I've added tests to test_source_git.py for the
retryFetch=True
case that fail, demonstrating the issue.The second commit fixes the failing tests.
Contributor Checklist:
newsfragments
directory (and read theREADME.txt
in that directory)