[monitoring] fix: use retrying module in the fixture class

[tmatsuo]

fixes #2971
fixes #2972
fixes #2973
fixes #3085

It will likely fix those issues, not guaranteed, but it's worth a try.

[kurtisvg]

Any reason not to pin a version?

[tmatsuo]

This is a temporary workaround. It'll go away.
(Actually, I'll use flaky, so those 2 lines will go away)

[kurtisvg]

In general I'd prefer we stop relying on this module altogether. It's not maintained and a lot of the logic isn't really great for testing.

IIRC, eventually_consistent just attempts the function until it passes. This could make the issue of failures during concurrent tests worse. There are other libraries (like flaky) that do this better and don't add extra maintenance on us.

[tmatsuo]

@kurtisvg
Flaky sounds like a plan. I'll use flaky for this test.

But, I kinda disagree with your two points; for the first point "It's not maintained", we can maintain it, right? I'm happy to do that from now on. For the second point, "lot of the logic isn't really great for testing." Can you elaborate? Maybe you can file a bug on the repo?

[tmatsuo]

I still think eventually_consistent is good for eventually consistent tests. It is basically a wrapper around retrying module with default exponential backoff parameters.

But I agree that flaky is more suitable with this test.

[kurtisvg]

RE: maintenance - it doesn't seem worth the effort of us maintaining and releasing a special module just for testing our samples. Are our libraries really so hard to test that we can't find an out of the box solution to reach reasonable tests? (I think the answer is no)

RE: logic - IMO, flakey tests are bad tests. Running a test several times hoping at least one passes means that potentially, our users also have to run the sample multiple times to make sure that it's working. Ideally, the snippet should work the first time, every time.

[tmatsuo]

@kurtisvg Thanks for the discussion. Let's discuss further off PR :)

[tmatsuo]

@kurtisvg Thanks, now it's using flaky. PTAL

[tmatsuo]

Ah, as far as I read the logs correctly, flaky doesn't retry.

Oh I just found the pytest marker. Another commit is on the way.

[tmatsuo]

Oh I think I started to understand the error better.
The log says:

ERROR at setup of test_list_alert_policies

This is failing in the fixture setup. I think I'm going to retry the setup with retrying module with an exponential backoffs.

[kurtisvg]

@kurtisvg Thanks, now it's using flaky. PTAL

@tmatsuo You may also want to want to to try a retry filter with a pause. Something like this:

def retry_on_concurrent_edits(err, *args):
    time.sleep(1)
    return "Too many concurrent edits to the project configuration" in str(err)

@flaky(rerun_filter=delay_rerun)
def test():

I'm not sure if fixtures are recreated on retry, but that should at least give a pause in between different attempts (and only retry if it's the 409 error).

[tmatsuo]

@kurtisvg Now I use retrying module in the fixture class.

I hope it will fix the concurrent tests. Let's see the kokoro result.

[tmatsuo]

Ah, interesting. The latest retrying module doesn't support the tuple. I'll make a fix.

[tmatsuo]

Alright, the tests are now passing. Previously, one of the tests was constantly failing, so it's certainly an improvement. I'll re-run the tests few times to see how stable they are.

Note: this is not an ultimate fix because when we have more and more builds, it will cause problems. Also, the presubmit builds might conflict with continuous builds, etc etc.

I'll file another issue for the fundamental fix.

@kurtisvg PTAL

[tmatsuo]

Tests were all green 3 times in a row, but now we have another faiulre.

It seems like we need to retry in the test itself, and the teardown. Another commit is on the way.

[tmatsuo]

Tests are all green 5 times in a row.
@leahecole PTAL

[leahecole]

oh awesome love that we're moving to retry instead