Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/129/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/129/console

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/130/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/130/console

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/131/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/131/console

Build has failed

14:29:03  + git fetch -n https://forge.softwareheritage.org/source/staging.git +refs/tags/phabricator/diff/10892:diff-target +refs/tags/phabricator/base/10892:diff-base
14:29:04  fatal: Couldn't find remote ref refs/tags/phabricator/diff/10892

I have no idea what's jenkins is babbling about
¯\_(ツ)_/¯

I did not change anything regarding diff creation or update:

• creation: arc diff origin/master
• update: arc diff origin/master --update D3060

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/133/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/133/console

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/134/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/134/console

Build has FAILED

Link to build: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/135/
See console output for more information: https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/135/console

Why did you remove assertions?

Well, for 1, it's already checked through the has_calls assertions just
before those. For 2, techically, i don't know how to check futher that
assertion since it's no longer a mock with what i propose.

In D3060#74554, @ardumont wrote:

Well, for 1, it's already checked through the has_calls assertions just
before those.

No, it checked the methods were called, but not that sleep was called between these calls

For 2, techically, i don't know how to check futher that
assertion since it's no longer a mock with what i propose.

Use a MagicMock instead of lambda x: None

No, it checked the methods were called, but not that sleep was called between these calls

Yes, but sleep is an implementation detail of the retry mechanism.
What's important here is that the methods are called the number of times they are.
And that is already tested with what i said ;)

If we don't want to test the sleeps, it means they are useless, so why do we have them?

If we don't want to test the sleeps, it means they are useless, so why do we have them?

No, it's not useless. We want the full blown retry behavior in production. We
don't want to be slowed by the exponential backoff in the tests though. They
are already quite numerous and long as it is.

Thus why you initially mock the sleep calls. Though, i argue the initial
assertions added at the same time the mocks were introduced were unneeded.

That's not something we implemented, that's something hidden beneath the retry
modules so no need for us to test it again. It's enough to assert it's tried 3
times which correspond to what we declare in the swh.storage.retry.swh_retry
decorator.

Also, note that i aligned those test implementations with other similar tests
in storage [1]

[1] https://forge.softwareheritage.org/D3010#inline-21159

Build is green

Patch application report for D3060 (id=10913)

Rebasing onto ecadd53737...

Current branch diff-target is up to date.

Changes applied before test

commit 49109d173f76c7a77e2000c323c70657226fb442
Author: Antoine R. Dumont (@ardumont) <antoine.romain.dumont@gmail.com>
Date:   Fri Apr 24 12:22:29 2020 +0200

    test_retry: Centralize time.sleep setup within a fixture
    
    This monkeypatches the internal sleep function used to not wait. This kept the
    previous behavior. It changes the implementation to monkeypatch though. This
    also centralizes within a fixture.
    
    This avoids repeating setup. The previous implementation cluttered the tests
    body instruction with internal implementation details.

See https://jenkins.softwareheritage.org/job/DSTO/job/tests-on-diff/136/ for more details.

Build is green

cool.

But i did not change anything in my ways...
I just don't know what changed...
¯\_(ツ)_/¯