st2client - elapsed time in a more user-friendly format

[winem]

This PR changes the output format of the elapsed time on st2 execution get.

Old: status: succeeded (76s elapsed) status: succeeded (7350s elapsed)
New: status: succeeded (01m16s elapsed) status: succeeded (2h2m30s elapsed)

Leading units with a value of 0 will be omitted.

fixes #4944

[arm4b]

Thanks for the PR!

Old: status: succeeded (76s elapsed)
New: status: succeeded (0:01:16 hours elapsed)

Looking at the #4944, I think what that Issue suggested is to provide more user-friendly and human-readable format. Converting 5s to hours doesn't make it easier to understand.

Here is an example what would make sense behind that idea:

• 30s remains 30s
• 339s becomes 5m39s
• 7350s becomes 2h2m30s

[winem]

Hi @armab, I updated the PR to present the elapsed time as requested.

[arm4b]

Thanks, that definitely looks more user-friendly! 👍

For some reason looks like you deleted the st2tests dir with existing tests, check the diff.
Can you please also update the initial PR description with the new design/behavior?

[winem]

Oh jesus, sorry! This explains a lot... will fix this shortly.

Update: already done. Something went wrong when I merged the upstream/master into my branch.

[Sheshagiri]

LGTM 👍

[arm4b]

Can we cover this change with some simple unit tests?

There are minor lint issues related to the new code in CI checks, - please take a look.

Additionally, please update the initial PR description with the new design/behavior.

[winem]

I fixed the multiple spaces before the = operator as reported by lint. That's something I'll keep in mind as I use this to format my code in other projects from time to time. So it shouldn't happen again on a st2 PR.

The PR description was updated as well.

I will provide a proper unit test in the next days as I'm working on st2 and other open source projects mostly in the evening after my usual job (which also explains my delayed reaction to the CI status from time to time. :) ).

Will mark this PR as draft until I provided working tests.

[winem]

I added a test but it looks like I got something wrong. My expectation was that the reported duration will be calculated from end_timestamp - start_timestamp but the duration is always 1 second (example: https://travis-ci.org/github/StackStorm/st2/jobs/698087456). I will investigate this tomorrow.

[winem]

Hey team,

I definitely need some help here. :( The unit test keeps failing unless I use mock.patch.object to mock the response which should not be needed. Example for the working test:

st2/st2client/tests/unit/test_formatters.py

Line 137 in c5d8e63

@mock.patch.object(

But as far as I can say (and @nmaludy agreed here on our recent slack discussion on this topic) it should not be required to use mock.patch.object to return the correct response.

The cause for the failing test is that the test always get's the default executions object with the duration of 1 second and not the expected execution_with_hours_in_elapsed_time.json with a duration of 2h0m1s as per:

"start_timestamp": "2014-12-02T19:56:06.900000Z",
"end_timestamp": "2014-12-02T21:56:07.000000Z",

So the question is, why argv = ['execution', 'get', DURATION_HOURS['id'], '--attr', 'status'] in my test on line 119 returns the start_timestamp and end_timestamp of EXECUTION = FIXTURES['executions']['execution.json'] instead of the one from DURATION_HOURS = FIXTURES['executions']['execution_with_hours_in_elapsed_time.json'].

(I also wrote a summary in the slack #development channel last week, just in case it is helpful in any way: https://stackstorm-community.slack.com/archives/CSBELJ78A/p1592686455460800)

Any help or hint is highly appreciated. I'd also be fine to use mock.patch.object again if anyone can explain why it is needed here, so that I can learn from it for my next PR.

Cheers,
Marcel

[cognifloyd]

Helpful FYI (a warning of sorts): We just merged the PR that reformatted the code with black. So, this needs to be rebased or master needs to be merged (resolve any conflicts & reformat with black).

As far as the unit test goes, I left a comment. All of the methods are employing mocked http responses, albeit indirectly for those that use self._get_execution. So, if you don't want to mock, you'll need to come up with alternative test machinery.

[cognifloyd]

I think using self._get_execution() is the source of the error because _get_execution() has a mock so it always returns the json from EXECUTION, but you need it to return DURATION_HOURS.

Here's _get_execution with the mocked EXECUTION response:

    @mock.patch.object(
        httpclient.HTTPClient,
        "get",
        mock.MagicMock(
            return_value=base.FakeResponse(json.dumps(EXECUTION), 200, "OK", {})
        ),
    )
    def _get_execution(self, argv):
        self.assertEqual(self.shell.run(argv), 0)
        self._undo_console_redirect()
        with open(self.path, "r") as fd:
            content = fd.read()

        return content

That's why your example unit test that uses mock.MagicMock worked better than self._get_execution().

[CLAassistant]

All committers have signed the CLA.