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

Express more tests via public API #11299

Closed
gaearon opened this issue Oct 20, 2017 · 133 comments
Closed

Express more tests via public API #11299

gaearon opened this issue Oct 20, 2017 · 133 comments

Comments

@gaearon
Copy link
Member

@gaearon gaearon commented Oct 20, 2017

This is a great contribution opportunity.
We need to rewrite more unit tests in terms of public API.

This means that they can only import npm entry points like react, react-dom, react-dom/test-utils, react-test-renderer, etc, but not internal modules like SyntheticEvent or ReactDOMComponentTree. The “bad” requires are already marked with a TODO in tests so you won’t miss them.

To help with this:

  1. Find // TODO: can we express this test with only public API? in the unclaimed test files below.
  2. Comment in this issue if you want to take a particular unit test file, with its name.
  3. Submit a PR that rewrites the test to use public APIs instead.

Step 3 requires some thinking. You can use previous examples where we rewrote tests with public API for inspiration. For example:

Generally, you need to think about how the behavior you’re testing actually reproduces in a React app, and then test for that. In rare cases it may involve exposing some API as public which we’ll need to discuss separately, so don’t hesitate to start a discussion! If you can’t figure out how to rewrite some particular test with a public API, comment here and we can brainstorm.

Here is the full list of tests that need to change. Some of them may be simple one-liner changes, some may involve a bit of a rewrite, some may require rewriting from scratch. Some may even be impossible, but research leading to that conclusion is still very valuable and we’d love to know that.

Try them and let us know:

Update: all tests are taken now. Subscribe to this issue! They might free up in the future if somebody doesn’t have the time to finish the work. We’ll comment if some test becomes available to try again.


First-time contributor? Refer to our contribution instructions.

Not clear how to fix a specific test? Comment with what you tried, and we can brainstorm.

If you gave up on some test, please post your findings in a comment so we can decide what to do next. It’s fine if you just didn’t find the time or couldn’t figure it out—we can try to help, and maybe somebody else can pick it up later.


@jeremenichelli
Copy link
Contributor

@jeremenichelli jeremenichelli commented Oct 20, 2017

I can take a look at this over the weekend and see if I can tackle it in the short term.

@gaearon
Copy link
Member Author

@gaearon gaearon commented Oct 20, 2017

Great, thanks! If you pick any particular test, please comment with the filename in the thread so that someone else doesn’t start working on the same test.

@silvestrijonathan
Copy link
Contributor

@silvestrijonathan silvestrijonathan commented Oct 20, 2017

I am definitely very interested in contributing to this. I will look through this weekend and find an opportunity to refactor!

@SadPandaBear
Copy link
Contributor

@SadPandaBear SadPandaBear commented Oct 20, 2017

I am interested as well 👍

@gaearon
Copy link
Member Author

@gaearon gaearon commented Oct 20, 2017

I published a list of tests in the first post. Just let me know which ones you’d like to take and I’ll assign them to you.

@SadPandaBear
Copy link
Contributor

@SadPandaBear SadPandaBear commented Oct 20, 2017

ReactDOMInput-test is fine with me :)

@gaearon
Copy link
Member Author

@gaearon gaearon commented Oct 20, 2017

@SadPandaBear You got it!

@reznord
Copy link
Contributor

@reznord reznord commented Oct 20, 2017

I can work on ReactErrorUtils-test. 😄

@silvestrijonathan
Copy link
Contributor

@silvestrijonathan silvestrijonathan commented Oct 20, 2017

I will take a look at setInnerHTML-test.js

@king0120
Copy link

@king0120 king0120 commented Oct 20, 2017

I'll do getEventCharCode-test.js. 😀

@michaelwilcox
Copy link
Contributor

@michaelwilcox michaelwilcox commented Oct 20, 2017

I can work on getEventKey-test.js.

@jeremenichelli
Copy link
Contributor

@jeremenichelli jeremenichelli commented Oct 20, 2017

I can take escapeTextContentForBrowser-test.js.

@Ethan-Arrowood
Copy link
Contributor

@Ethan-Arrowood Ethan-Arrowood commented Oct 20, 2017

I'd like to give ChangeEventPlugin-test.js a shot :)

@dleitee
Copy link

@dleitee dleitee commented Oct 20, 2017

I can take SyntheticEvent-test.js

@accordeiro
Copy link
Contributor

@accordeiro accordeiro commented Oct 20, 2017

I'd like to work on EnterLeaveEventPlugin-test.js

@enapupe
Copy link
Contributor

@enapupe enapupe commented Oct 20, 2017

I'd like to work on ReactDOMEventListener-test.js

@danilowoz
Copy link

@danilowoz danilowoz commented Oct 20, 2017

I would like to take BeforeInputEventPlugin-test.js

@aarboleda1
Copy link
Contributor

@aarboleda1 aarboleda1 commented Oct 20, 2017

I'd like to take SyntheticKeyboardEvent-test.js. Thanks 👍

@email2vimalraj
Copy link

@email2vimalraj email2vimalraj commented Oct 20, 2017

Let me take inputValueTracking-test.js

@douglasgimli
Copy link
Contributor

@douglasgimli douglasgimli commented Oct 20, 2017

I'd like to work on SyntheticWheelEvent-test.js

@bartsmykla
Copy link

@bartsmykla bartsmykla commented Oct 20, 2017

Can I take: ReactBrowserEventEmitter-test.js ?

@morajabi
Copy link

@morajabi morajabi commented Oct 20, 2017

I'm taking SelectEventPlugin-test.js

@GordyD
Copy link
Contributor

@GordyD GordyD commented Oct 20, 2017

I'd like to work on ReactDOMComponentTree-test.js

@gdevincenzi
Copy link

@gdevincenzi gdevincenzi commented Oct 20, 2017

I'd like to work on ReactTreeTraversal-test.js

@jstejada
Copy link

@jstejada jstejada commented Oct 20, 2017

hi! 👋 I'd like to work on ReactCoroutine-test.js

anushreesubramani added a commit to anushreesubramani/react that referenced this issue Dec 7, 2017
 * Rewrite tests using only public API.
 * Modified the tests to prevent duplication of code.
 * Code review changes implemented.
 * Removed the .internal from the test file name as
   its now written using public APIs.
gaearon added a commit that referenced this issue Dec 7, 2017
*  ValidateDOMNesting tests(#11299)

 * Rewrite tests using only public API.
 * Modified the tests to prevent duplication of code.
 * Code review changes implemented.
 * Removed the .internal from the test file name as
   its now written using public APIs.

* Remove mutation

* Remove unnecessary argument

Now that we pass warnings, we don't need to pass a boolean.

* Move things around a bit, and add component stack assertions
@gaearon
Copy link
Member Author

@gaearon gaearon commented Dec 7, 2017

One more down! #11742

@reznord We haven't heard from you yet—did you start anything? If you're too busy maybe it's better to give someone else a chance to try.

Ethan-Arrowood added a commit to Ethan-Arrowood/react that referenced this issue Dec 8, 2017
…facebook#11309)

* Rewrite tests depending on internal API

* Remove focus being called when there was no blur event function before

* Remove triggering function and just let ReactTestUtils take care

* Use native events

* Remove duplicate

* Simulate native event when changing value on reentrant

* Change wasn't being called

* Use Simulate only

* Use React event handlers on test

* Move commentary

* Lint commit

* Use native event

* Comment native event dispatching

* Prettier

* add setUntrackedValue

* Prettier-all

* Use dispatchEvent instead of ReactTestUtils Simulates;

* Prettier

* Fix lint

* Remove useless arg

* Wrap event dispatcher into function

* Remove deprecated Event

* Remove unused change event dispatcher

* Fix merge

* Prettier

* Add missing focus/blur calls

They are necessary to cover for the fix in facebook#8240.
Ethan-Arrowood added a commit to Ethan-Arrowood/react that referenced this issue Dec 8, 2017
* Use only public API for EnterLeaveEventPlugin Tests (facebook#11299)

* Trigger native event to test EnterLeaveEventPlugin (facebook#11299)

* Rewrite EnterLeaveEventPlugin tests to use dispatchEvent

* Update EnterLeaveEventPlugin test to use OnMouseLeave event

* Add coverage for onMouseEnter too
Ethan-Arrowood added a commit to Ethan-Arrowood/react that referenced this issue Dec 8, 2017
…#11317)

* Add flow annotation to getEventKey.

* Remove Simulate and SimulateNative for native events.

* Style nits

* Oops
Ethan-Arrowood added a commit to Ethan-Arrowood/react that referenced this issue Dec 8, 2017
…11299) (facebook#11367)

* Update SyntheticWheelEvent tests to use public API

* Replace: ReactTestUtils.SimulateNative to native Events()

* Update: Replaced WheelEvent() interface to document.createEvent

* Fix: Lint SyntheticWheelEvent file

* Update: Custom WheelEvent function to a generic MouseEvent function

* Update: Prettier SyntheticWheelEvent-test.js

* Verify the `button` property is set on synthetic event

* Use MouseEvent constructor over custom helper

* Rewrite to test React rather than jsdom

* Force the .srcElement code path to execute

* Style tweaks and slight code reorganization
Ethan-Arrowood added a commit to Ethan-Arrowood/react that referenced this issue Dec 8, 2017
…acebook#11383)

* Rewrite ReactDOMComponentTree-test to test behavior using Public API

 - Part of facebook#11299
 - I've tried to identify cases where code within ReactDOMComponentTree is exercised and have updated accordingly but I'm not entirely sure whether I'm on the right track. I thought I'd PR to get feedback from the community. Looking forward to comments.

* Prettier and lint changes

* Remove testing of internals and add test cases for testing behavior exhibited after use of getInstanceFromNode

* [RFC] Update testing approach to verify exhibited behavior dependent upon methods in ReactDOMComponentTree

* Remove tests from event handlers and use sync tests

* Prettier changes

* Rename variables to be more semantic

* Prettier updates

* Update test following review

 - Use beforeEach and afterEach to set up and tear down container element for use in each test
 - Move any functions specific to one test to within test body (improves readability imo)

* Add coverage for getNodeFromInstance and implementation of getFiberCurrentPropsFromNode
 - After researching usage of getNodeFromInstance we can test getNodeFromInstance dispatching some events and asserting the id of the currentTarget
 - After checking git blame for getFiberCurrentPropsFromNode and reading through facebook#8607 I found a test that we can simplify to assert behavior of the function by ensuring event handler props are updated from the fiber props. Swapping out the implementation of this function with `return node[internalInstanceKey].memoizedProps` results in a failure.
@gaearon
Copy link
Member Author

@gaearon gaearon commented Dec 14, 2017

Ping @reznord

@vikramcse
Copy link

@vikramcse vikramcse commented Dec 29, 2017

Hi @gaearon, Can I take ReactErrorUtils-test.js for my first contribution?

@vikramcse
Copy link

@vikramcse vikramcse commented Jan 2, 2018

hi, @gaearon is there something that I can do in ReactErrorUtils-test.js

@madeinfree
Copy link
Contributor

@madeinfree madeinfree commented Jan 4, 2018

Hi, Does anyone can help me to continue the ReactBrowserEventEmitter-test.js test ? Because I'll busy on my working now no time to keep going, the PR is #11713, thank you so much !!

@gaearon
Copy link
Member Author

@gaearon gaearon commented Aug 15, 2018

Thanks to everyone again! There are still a few outstanding tests but I think we can close it.

@abhaynikam
Copy link
Contributor

@abhaynikam abhaynikam commented Sep 7, 2018

@gaearon : ReactErrorUtils-test.js I would like to work on it. can you confirm if I can pick that up?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.