Actions
Bug #4227
closed[Workbench] Pipeline elapsed time is misformatted
Status:
Resolved
Priority:
Normal
Assigned To:
Radhika Chippada
Category:
Workbench
Target version:
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 9 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 9 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 9 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 9 years ago
Thanks for following through on this. 99ad159 is good to merge, thanks.
Updated by Radhika Chippada over 9 years ago
- Status changed from In Progress to Resolved
Applied in changeset arvados|commit:6eaaae29a7af005e417673d79e0951122065e685.
Actions