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

Use Performance API to add "User Timings" #1296

Merged
merged 5 commits into from
Sep 19, 2018

Conversation

Turbo87
Copy link
Member

@Turbo87 Turbo87 commented Jun 29, 2018

bildschirmfoto 2018-06-29 um 16 35 07

I had implemented a similar thing using the on() event handlers for one of our projects and it was invaluable in tracking down slowness in our test suite. Having this implemented in QUnit by default makes it even more accessible for people that don't know about this API yet. :)

/cc @trentmwillis

Copy link
Contributor

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

I like it!

Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

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

Haven't done a full review, but I like this idea! A couple thoughts:

  1. Looks like the Performance API has started to land in newer Node versions. It would be good if we supported that use case as well. Any issues with adding support for that?

  2. Do you have any thoughts on adding tests for these? On the one hand, any tests are likely to feel like they're testing implementation details, but if users start querying the Performance marks/measurements, then we want to make sure we don't break them.

@Turbo87
Copy link
Member Author

Turbo87 commented Jun 30, 2018

Yeah, looks like node could also be supported, but I'd rather land this first and add node support in a subsequent PR. Tests would be nice, but we'd have to mock the performance API, and I'm not sure if it's worth it 🤔

@Turbo87
Copy link
Member Author

Turbo87 commented Jul 3, 2018

@trentmwillis I have a hard time finding any tests for the reports classes that I could extend. Did I miss anything? Do they exist?

@Turbo87
Copy link
Member Author

Turbo87 commented Aug 27, 2018

@trentmwillis friendly reminder :)

@gabrielcsapo
Copy link
Contributor

@trentmwillis bump

@trentmwillis
Copy link
Member

Sorry y'all, I've been busy. I'll take a look at this sometime this week. Adding a reminder so I don't forget...

Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

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

The reports are tested indirectly via the "events" tests.

After reviewing again, I'm fine with landing this as-is. We can add Node support and tests in follow-up PRs.

Thanks! And sorry for taking so long to merge

@trentmwillis trentmwillis merged commit 4b3cb56 into qunitjs:master Sep 19, 2018
@Turbo87 Turbo87 deleted the user-timing branch September 19, 2018 23:02
@Turbo87
Copy link
Member Author

Turbo87 commented Sep 19, 2018

awesome, thank you @trentmwillis :)

@Turbo87
Copy link
Member Author

Turbo87 commented Oct 9, 2018

@trentmwillis any plans on a release in the near future?

@trentmwillis
Copy link
Member

@Turbo87 yep, within the next day. See #1311 (comment)

@Turbo87
Copy link
Member Author

Turbo87 commented Oct 9, 2018

Great, thanks a lot!

@stefanpenner
Copy link
Contributor

awesome!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants