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.

Sign up for GitHub

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

Jump to bottom

added isProfiling() to ReactDebugTool and isRunning() to PerfTools #6763

Merged
merged 3 commits into from
May 13, 2016
Merged

added isProfiling() to ReactDebugTool and isRunning() to PerfTools #6763

merged 3 commits into from
May 13, 2016

Conversation

nfcampos
Copy link

I'm not sure the tests are added are worth it,

closes #6762

@nfcampos
Copy link
Author

nfcampos commented May 12, 2016 •
edited
Loading

should I add isRunning to the docs? is it this file?
https://github.com/facebook/react/blob/master/docs/docs/10.9-perf.md

@nfcampos nfcampos mentioned this pull request May 12, 2016
Closed
@nfcampos
Copy link
Author

the build timed out for some reason, not really sure why

gaearon
gaearon reviewed May 13, 2016
View reviewed changes
src/isomorphic/__tests__/ReactPerf-test.js
@@ -283,4 +285,12 @@ describe('ReactPerf', function() {
ReactPerf.printDOM(measurements);
expect(console.error.calls.length).toBe(1);
});

it('returns isRunning state', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be better to test this by running start() and stop().

Copy link
Author

Choose a reason for hiding this comment

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

I've changed it

@ghost
Copy link

ghost commented May 13, 2016

@nfcampos updated the pull request.

@gaearon
Copy link
Collaborator

gaearon commented May 13, 2016

Let’s add extra tests that verify that running begin/start or end/stop twice has no effect.

@gaearon gaearon added this to the 15.1.0 milestone May 13, 2016
@nfcampos
Copy link
Author

separate tests right? because they're testing start etc. instead of isRunning

@gaearon
Copy link
Collaborator

gaearon commented May 13, 2016

separate tests right? because they're testing start etc. instead of isRunning

Yeah. I’d probably just test the public API here (ReactPerf). Basically to verify that it doesn’t crash when you try to start() or stop() it twice, and that isRunning() returns a sensible value.

@nfcampos
Copy link
Author

@gaearon I've added those two tests to ReactPerf

@ghost
Copy link

ghost commented May 13, 2016

@nfcampos updated the pull request.

@gaearon gaearon merged commit 5569d1d into facebook:master May 13, 2016
@gaearon
Copy link
Collaborator

gaearon commented May 13, 2016

Thanks!

@nfcampos nfcampos deleted the is-running branch May 13, 2016 20:03
@zpao zpao modified the milestones: 15.1.0, 15.y.0 May 20, 2016
@zpao zpao modified the milestones: 15.y.0, 15-next Jun 1, 2016
zpao pushed a commit to zpao/react that referenced this pull request Jun 8, 2016
@gaearon @zpao
Merge pull request facebook#6763 from nfcampos/is-running
af0a915 
added isProfiling() to ReactDebugTool and isRunning() to PerfTools
(cherry picked from commit 5569d1d)
zpao pushed a commit that referenced this pull request Jun 14, 2016
@gaearon @zpao
Merge pull request #6763 from nfcampos/is-running
a88d821 
added isProfiling() to ReactDebugTool and isRunning() to PerfTools
(cherry picked from commit 5569d1d)
@zpao zpao modified the milestones: 15-next, 15.2.0 Jun 14, 2016
@renovate renovate bot mentioned this pull request Feb 2, 2018
Closed
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
CLA Signed
Projects
None yet
Milestone
15.2.0
Development

Successfully merging this pull request may close these issues.

ReactPerf 15.1.0-alpha.1 expose isProfiling on the exported object?
3 participants
@nfcampos @gaearon @zpao