Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

test: pretty test output #1085

Merged
merged 1 commit into from
Feb 26, 2018
Merged

Conversation

davisjam
Copy link
Contributor

Description

Problem:
On hangs, test output is hard to read.

Solution:
Print test name before starting.

Bonus:
Also print an approximation of how long each test takes to run.

Fixes #1084.

Submitter

  • All tests pass (CI should take care of this, once in place).
  • All lint checks pass (CI should take care of this, once in place).
  • Tests
    • No tests required for this PR.
  • Is release:
    • Version in package.json has been updated (see RELEASE.md).
    • The marked.min.js has been updated; or,
    • release does not change library.

Reviewer

@joshbruce @UziTech

Problem:
On hangs, test output is hard to read.

Solution:
Print test name before starting.

Bonus:
Also print an approximation of how long each test takes to run.

For markedjs#1084.
@joshbruce
Copy link
Member

I or @UziTech (most likely) would like to maybe start contributing directly to this PR - unless you would like to continue working on it yourself.

Therefore, I'm curious, would you be okay with us working directly on it; or, is the intention for us to review it and you update it??

@joshbruce joshbruce requested a review from UziTech February 26, 2018 22:24
@davisjam
Copy link
Contributor Author

Additional contributions here are welcome. I do not plan to make further additions to this PR.

I'll keep poking at #1083 as needed.

@joshbruce
Copy link
Member

Actually, now that I've actually done the review, seems like you've covered all the bases and then some (whitespacing over). Well done!

// returns time to millisecond granularity
function prettyElapsedTime(hrtimeElapsed) {
var seconds = hrtimeElapsed[0];
var fracInMs = Math.round(hrtimeElapsed[1] / 1e6);
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how hard up we are for the ms instead of seconds. Can we go to the second or third floating point??

0.001

@joshbruce
Copy link
Member

LGTM! Not a lot of possibility for negative impact. Merging.

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.

2 participants