Added request_interval feature to wait_for_successful_query module

[Oloremo]

What does this PR do?

Added request_interval kwargs to the http.wait_for_successful_query module to be aligned with the state.

What issues does this PR fix or reference?

#53738

Previous Behavior

request_interval was not supported in the http.wait_for_successful_query module

New Behavior

request_interval now supported in the http.wait_for_successful_query module

Tests written?

Yes

Commits signed with GPG?

Yes

[gtmanfred]

Small question

[gtmanfred]

Why not just put an elif here instead of else and then an if inside it?

[Oloremo]

I just copy-pasted this section from the state file. Keeping them the same sounds reasonable to me OR we could change them both...

[gtmanfred]

I would change both, I don't see a reason to not use elif.

…

On Sat, Jul 6, 2019, 9:03 AM Proskurin Kirill ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In salt/modules/http.py <#53739 (comment)>: > @@ -79,6 +79,10 @@ def wait_for_successful_query(url, wait_for=300, **kwargs): raise caught_exception # pylint: disable=E0702 return result + else: I just copy-pasted this section from the state file. Keeping them the same sounds reasonable to me OR we could change them both... — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#53739?email_source=notifications&email_token=AAFSZIM3VH2YLM76UNW2KR3P6CQ3HA5CNFSM4H6TKUQ2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB5U4N5A#discussion_r300832223>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAFSZINQ4XHK2RUEGA5BTPTP6CQ3HANCNFSM4H6TKUQQ> .

[Oloremo]

I would change both, I don't see a reason to not use elif.

done

[DmitryKuzmenko]

@Oloremo, thank you for your patch.
The module unit test could be simply implemented by mocking the query function with return=[{'error': 'error'}, {}] and mock the time.sleep. Then call the wait_for_successful_query with request_interval set to a non-zero value and assert time.sleep mock called once with the value. Similar is for the state module test.

[Oloremo]

@DmitryKuzmenko I gave it a thought and still not sure how to implement it.
The main problem here is while True loop which could only be broken by the time check. And yet I need to pass the time check once to reach elif clause and check the logic but on a second iteration of the loop I need to break the loop somehow. And I have no idea how since if I mock time.time I'll never get out of the loop.

[DmitryKuzmenko]

@Oloremo you don't need to mock time.time. If you're mocking query with MagicMock(return=[{'error': 'error'}, {}]) and time.sleep with MagicMock() you'll have the following:

    while True:
        caught_exception = None
        result = None
        try:
            result = query(url=url, **kwargs)
            # ITERATION 1: result = {'error': 'error'}
            # ITERATION 2: result = {}
            if not result.get('Error') and not result.get('error'):
                # ITERATION 2: returning the result
                return result
        except Exception as exc:
            caught_exception = exc

        if time.time() > starttime + wait_for:
            if not result and caught_exception:
                # workaround pylint bug https://www.logilab.org/ticket/3207
                raise caught_exception  # pylint: disable=E0702

            return result
        elif 'request_interval' in kwargs:
            # ITERATION 1: calling sleep
            # Space requests out by delaying for an interval
            time.sleep(kwargs['request_interval'])

It will exit on iteration 2 and called time.sleep once with the value of request_interval kwarg that you can check using the magic mock.

[Oloremo]

@DmitryKuzmenko

you don't need to mock time.time
It will exit on iteration 2

It won't unless we wait for wait_for with a minimum of 1 sec, and since we mock time.sleep() it could do a lot of loops during this one sec... Is it ok?

Or am I misunderstanding something?..

[Oloremo]

Oh wait, you're saying that in iteration 2 result = {} but how can I make different result for a second iteration if it's already mocked?

EDIT: Oh looks like I get it. MagicMock mocks the iterator with a list of results. Didn't know that, will try to implement!

[Oloremo]

Test added. Please do check if they make sense.

[Oloremo]

Looks like failed jenkins/pr/py3-windows-2016 test has nothing to do with my changes. Not sure what to do here.

[Oloremo]

Now tests failing on boto stuff...

[waynew]

@Oloremo FWIW we've got an intense focus to stabilize our test suite/pipeline - you can feel free to investigate failures if you want to try to get this merged sooner, or alternatively after we've stabilized things it should be much easier to get merged in.

[Oloremo]

@waynew Well, I'd like to help but I have no idea how your CI works and have no idea what is wrong with the test I mentioned. :-(

[waynew]

We're running on Jenkins, using a tool called kitchen - you should be able to view most of the information available there if you click on the details of the builds.

If you check out the console logs, you can see the kitchen commands that we usually run. I don't have the time right now to put up some walkthroughs, unfortunately. If there are issues that crop up though, I'll do what I can - there's also the Salt community Slack and IRC where you might be able to find others who can assist.

[Oloremo]

re-run all

[Oloremo]

re-run full all

[waynew]

re-run full all

[Oloremo]

it seems re-run is not working. :-)

[waynew]

I was thinking that it may not be active on this branch. If you rebase against 2018.3 and force push to your branch, that will re-run the tests... or something has gone terribly wrong 😛

[Oloremo]

From a quick glance none of failed test related to the change. No idea how to pass tests. :-(

[waynew]

@Oloremo given the changes announced this morning, would you mind rebasing these changes against master and editing this PR to target master?

[Oloremo]

@waynew Sure, gimme a few days

[Oloremo]

deprecated by #54916