Skip to content

Conversation

@LennDG
Copy link

@LennDG LennDG commented Feb 1, 2018

PR for the show duration issue.

Adds support for duration display in a readable format.

@LennDG LennDG mentioned this pull request Feb 1, 2018
@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 98.88% when pulling bb45ff1 on LennDG:master into 1cae8ea on wswebcreation:master.

@wswebcreation
Copy link
Collaborator

Hi @LennDG

Tnx for the PR. This looks already nice. I think we also need to update the other templates (custom-meta templates). I'll also think about a the time in the overview page. Total time can't be calculated because we don't know if the tests have been executed in parallel.

I only need to check when I have time to dive into this because I have a busy weekend and week, but I'll come back on it. Again tnx!

@LennDG
Copy link
Author

LennDG commented Feb 2, 2018

The custom templates are only for the features overview page (that one might want duration so you can see the duration on the overview page) and for the metadata overview on a feature page, but on that page the duration is already displayed below the chart.
So unless the features overview page should have the duration, the custom templates don't need to be adjusted.
I indeed do not display total time (although the functionality was in the code) since it is not very useful in many cases.

Copy link
Collaborator

@wswebcreation wswebcreation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @LennDG,

I finally got time ti review your PR, I just got settled in my new home.

This PR looks good, I do have some feedback. I can also push the changes if you don't have time

m = timeData.m ? timeData.m + "m" : "";
h = timeData.h ? timeData.h + "h" : "";
d = timeData.d ? timeData.d + "d" : "";
return d + h + m + s + ms;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've been thinking about this and I wonder what the best layout would be, or yours

image

or this

image

Personally I prefer the last, even though there is no d/h/m/s/ms notation in it.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is fine by me, I didn't think of different ways to display and just used the format that the Cucumber Jenkins Plugin uses.

});
}

// From https://gist.github.com/remino/1563878
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For calculating times I'd prefer using a library to keep this code as clean as possible, I don't want to make unit test for time calculations, they always go wrong. We can use momentjs for it and solve the time calculation with 3 lines of code and a require at the top of the file.

const moment = require('moment');


function formatDuration(ms) {
    return moment.utc(ms).format("HH:mm:ss.SSS")
}

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I didn't want to add extra dependencies with my PR. Using a library will save some headaches.


You can change the report name to a name you want

### `displayDuration`
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 for adding this as an option!!

- **Mandatory:** No

If set to true the duration of steps, scenarios and features is displayed on the Feature page in an easily readable format.
This expects the durations in the report to be in milliseconds, which might result in incorrect durations when using a version of Cucumber that does not report in milliseconds (like v.1).
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@LennDG
Copy link
Author

LennDG commented Feb 20, 2018

Overall you can just push these changes since I have nothing more to add. Looking forward to using the official package again!

@wswebcreation wswebcreation merged commit bb45ff1 into WasiqB:master Feb 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants