Actions
Bug #4227
closed[Workbench] Pipeline elapsed time is misformatted
Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
Workbench
Target version:
Start date:
10/21/2014
Due date:
% Done:
100%
Estimated time:
(Total: 0.00 h)
Story points:
0.5
Description
In pipeline_instances/qr1hi-d1hrv-43y08obrh2vetuv, it says This pipeline started at 11:56 AM 10/16/2014. It failed after 4 minutes 52 seconds at 12:01 AM 10/16/2014.
I expected it to say 12:01 "PM"
Updated by Brett Smith over 10 years ago
- Subject changed from Time log is wrong to [Workbench] Pipeline elapsed time is misformatted
- Category set to Workbench
Updated by Radhika Chippada over 10 years ago
- Status changed from New to In Progress
- Assigned To set to Radhika Chippada
- Target version set to 2014-10-29 sprint
The issue is in dates.js, another one of the under-tested javascript areas. Fix issue and add tests.
Updated by Brett Smith over 10 years ago
Reviewing e102efba. The fix is good, but I'm surprised at how involved the test code is.
- I'm not sure I understand what value we get out of testing ten cases of this. It seems like we get the most value just by testing a simple case, a case that spans the AM/PM boundary, and a case that spans the midnight boundary. If there are other cases that have some unique property, then let's definitely include those. But ten seems like an arbitrary number, and that's a lot of overhead given that we're firing up a whole browser session to test datetime formatting.
- I now limited to two tests, one with 0 run time and one with run time that spans between AM and PM
- You can use DateTime::strptime to parse these strings, rather than doing all the splitting and array indexing yourself.
- Thanks for this tip. It is fantastic
- Rather than trying to parse the "elapsed time" string, let's use fixtures whose elapsed time we know ahead of time, and compare against that. That will save us a bunch of parsing and math that aren't directly relevant to what we're trying to test (the datetime formatting).
- Adjusted this
BTW, there's also trailing whitespace on some of the lines in the current branch.
Thanks.
Updated by Brett Smith over 10 years ago
Thanks for following through on this. 99ad159 is good to merge, thanks.
Updated by Radhika Chippada over 10 years ago
- Status changed from In Progress to Resolved
Applied in changeset arvados|commit:6eaaae29a7af005e417673d79e0951122065e685.
Actions