st2 icon indicating copy to clipboard operation
st2 copied to clipboard

st2client - elapsed time in a more user-friendly format

Open winem opened this issue 4 years ago • 6 comments

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

winem avatar Jun 06 '20 22:06 winem

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

winem avatar Jun 07 '20 19:06 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.

winem avatar Jun 07 '20 20:06 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 avatar Jun 08 '20 19:06 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 avatar Jun 14 '20 00:06 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: https://github.com/StackStorm/st2/blob/c5d8e63eae7ba69f2e39caac598161194c3899e3/st2client/tests/unit/test_formatters.py#L137

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

winem avatar Jun 26 '20 20:06 winem

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Sep 06 '21 14:09 CLAassistant