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

Run each test in its own <iframe> #1

Merged
merged 9 commits into from
Jun 3, 2013

Conversation

benjamn
Copy link
Contributor

@benjamn benjamn commented May 29, 2013

This is not blocking the initial launch, so feel free to put it on the back-burner for now.

The Jasmine test harness still runs in the parent window and reports to PhantomJS via window.callPhantom, but each test <iframe> has its own copy of react-test.js and each individual test module is required in the global context of a separate <iframe>.

This gives us a significant approximation of the benefits of mocking, at least in terms of isolating tests from one another.

cr @jeffmo @zpao

Fp.bind = function(context) {
var func = this;
var args = slice.call(arguments, 1);
return args.length > 0 ? function() {
Copy link
Member

Choose a reason for hiding this comment

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

ternary across this many lines is groosssss. Can you just use if?

What about just using the shim @ https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Function/bind - this here is not quite the same.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, sure.

For what it's worth, the static_upstream polyfill doesn't bother with constructor function binding, either. Seems like a nontrivial cost to pay for a minority use case.

@zpao
Copy link
Member

zpao commented Jun 3, 2013

A++, merge away when you're ready

@petehunt
Copy link
Contributor

petehunt commented Jun 3, 2013

I like this much better than our current test system.

But isn't it masking the fact that dumpCache() doesn't work consistently between our phantom and jstest test runners? I think that's why we had the original issue where the reactRoot count was not reset when running in phantom. Doesn't that mean that some tests will pass in phantom (because they're better isolated) and fail in jstest?

@benjamn
Copy link
Contributor Author

benjamn commented Jun 3, 2013

@petehunt good question. I believe jstest actually creates a new window environment for each test, which would mean that the <iframe> strategy is as close as we can get in a real browser: https://phabricator.fb.com/diffusion/E/browse/tfb/trunk/www/scripts/third_party/jstest/runner/run-single-jstest.js

So I think/hope that we will have fewer disagreements between phantom and jstest.

It's still masking the fact that dumpCache doesn't work the same way, yes. But making dumpCache work would require rewriting the module system that Browserify uses. That's not out of the question by any means, but right now it's nice to be testing the same imperfect module system that we ship in production Browserify bundles.

cc @jeffmo who might know better

benjamn added a commit that referenced this pull request Jun 3, 2013
Run each test in its own <iframe>
@benjamn benjamn merged commit e829f8f into facebook:master Jun 3, 2013
@petehunt
Copy link
Contributor

petehunt commented Jun 3, 2013

👍

@subtleGradient subtleGradient mentioned this pull request Nov 6, 2013
jimfb pushed a commit that referenced this pull request Apr 8, 2015
Update ReactCompositeComponentNestedState-test.js
Jyrno42 added a commit to Jyrno42/react that referenced this pull request Sep 9, 2015
Jyrno42 added a commit to Jyrno42/react that referenced this pull request Sep 9, 2015
defan-marunchak pushed a commit to defan-marunchak/react that referenced this pull request Feb 3, 2016
gaearon pushed a commit that referenced this pull request Nov 4, 2016
[Fiber] Refactor error boundaries in Fiber
acusti pushed a commit to brandcast/react that referenced this pull request Mar 15, 2017
gaearon pushed a commit that referenced this pull request Aug 4, 2017
* Remove PooledClass from FallbackCompositionState

The only module that uses FallbackCompositonState is BeforeInputEventPlugin. The way its structured means there can only be a single instance of FallbackCompositionState at any given time (stored in a local variable at the top-level) so we don't really need pooling here at all. Instead, a single object is now stored in FallbackCompositionState, and access (initializing, reseting, getting data) is gaurded by the exported helper object.

* Use new FallbackCompositionState API in BeforeInputEventPlugin

* Implement event-specific pooling in SyntheticEvent

* Remove PooledClass from TopLevelCallbackBookKeeping

* Update results.json

* Add pooled event test fixtures (#1)

* Fix fixture lint
klamping referenced this pull request in klamping/react Mar 9, 2018
@sebmarkbage sebmarkbage mentioned this pull request Feb 11, 2019
11 tasks
@NE-SmallTown NE-SmallTown mentioned this pull request Apr 28, 2019
yvettep321 pushed a commit to yvettep321/react that referenced this pull request Jan 11, 2021
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