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

Add test exercising public API to test BeforeInputEventPlugin + FallbackCompositionState #11849

Merged
merged 9 commits into from Feb 22, 2018

Conversation

GordyD
Copy link
Contributor

@GordyD GordyD commented Dec 14, 2017

Part of the effort to refactor tests to use the Public API #11299

  • I've adopted a similar approach to the existing test for BeforeInputEventPlugin
  • These tests simulate events and then assert the event handler for onBeforeInput is fired or not fired based on the test conditions
  • The test scenarios are tested against IE11, Webkite and Presto environment simulations
  • I've encorporated what I understand to be the functionality in the FallbackCompositionState test

There are a few more test cases to add. However, the main reason I'm putting this diff up is to:

  • get feedback on approach
  • get community support on resolving a failing test I'm encountering when testing the expected behavior of the compositionend fallback behavior on a contenteditable - right now I avoid running these test cases by avoiding the failing tests when simulating an environment which relies on the fallback statetestContentEditableComponent(environments[2], scenarios.splice(0, 5));

@gaearon
Copy link
Collaborator

gaearon commented Jan 5, 2018

The approach seems fine to me, we can always refactor later if needed.

I'm way out of my depth re: the second question so I hope you can carry this through and figure it out.

@GordyD
Copy link
Contributor Author

GordyD commented Jan 16, 2018

Update: Will circle back around onto this at some point this week.

@gaearon
Copy link
Collaborator

gaearon commented Jan 16, 2018

Awesome, thank you!

@GordyD
Copy link
Contributor Author

GordyD commented Jan 28, 2018

Finally got the chance to look back into this. After rebasing onto master it looks like support for browsers using the presto engine (Opera Mini) have been removed so I've removed any specific testing for that browser in my tests too. I still need to simulate a similar environment though (no composition event support) to test the behavior of FallbackCompositionState so I've left a test case in for that. (I've omitted testing FallbackCompositionState on a contenteditable though as the fallback behavior for triggering compositionstart does not work as expected - keydown and keyup events are mapped as keypress events(??) - which means the fallback logic will not be triggered - not sure if this a blocker to accepting this or not)

My approach:

I went through the following steps to check out coverage of the tests I've written whilst watching the tests:

  • comment out 'BeforeInputEventPlugin' in DOMEventPluginOrder causes all tests to fail
  • comment out blocks of in BeforeInputEventPlugin to check for corresponding failing tests
  • comment out blocks in FallbackCompositionState to check for corresponding failing tests

Things of note in BeforeInputEventPlugin.js:

  • I can comment out the body of getCompositionEventType, or comment out line 207, with no failing tests - it seems under the conditions where this function is called - the extraction of a onBeforeInput event is handled is by both the underlying extraction functions - so either I need another test case to exercise this path, or this code is redundant?
  • I can comment out lines 226-228 with no failing tests even though the code path is exercised - I wonder if this redundant or not.
  • commenting out accumulateTwoPhaseDispatches in either extractXXX function does not break my tests - I'm not sure how to protect against this - any pointers would be welcome here :)

@gaearon any thoughts on how to proceed? I think we have pretty decent coverage here with testing through the public API.

N.B I've also removed the no longer needed tests exercising the internal API.

…ackCompositionState

 - I've adopted a similar approach to the existing test for BeforeInputEventPlugin
 - I've simulated events and then assert the event handler for onBeforeInput is fired or not fired based on the test conditions
 - The scenarios are tested against IE11, Webkite and Presto environment simulations
 - I've encorporated what I understand to be the functionality in the FallbackCompositionState test
…is not supported in Presto powered browsers (Opera).
…led in BeforeInputEventPlugin.

We still need to exercise usage of FallbackCompositionState though so let's keep a test where the env does not support Composition and Text events.
The BeforeInputEventPlugin is responsible for emitting these events so we need to add tests for this. This also ensure we exercise the code path that, L207, that was not previously exercised with the public tests.
@gaearon gaearon merged commit cf58f29 into facebook:master Feb 22, 2018
@gaearon
Copy link
Collaborator

gaearon commented Feb 22, 2018

LGTM, thanks!

LeonYuAng3NT pushed a commit to LeonYuAng3NT/react that referenced this pull request Mar 22, 2018
…ackCompositionState (facebook#11849)

* Add test exercising public API to test BeforeInputEventPlugin + FallbackCompositionState

 - I've adopted a similar approach to the existing test for BeforeInputEventPlugin
 - I've simulated events and then assert the event handler for onBeforeInput is fired or not fired based on the test conditions
 - The scenarios are tested against IE11, Webkite and Presto environment simulations
 - I've encorporated what I understand to be the functionality in the FallbackCompositionState test

* Prettier

* Linting fixes

* Remove test for contenteditable in Presto - the contenteditable type is not supported in Presto powered browsers (Opera).

* Remove mention of Presto as this explicit condition is no longer handled in BeforeInputEventPlugin.

We still need to exercise usage of FallbackCompositionState though so let's keep a test where the env does not support Composition and Text events.

* Add tests for envs with only CompositionEvent support

* Remove internal tests no longer needed

* Shorten test case names to satisfy lint rules

* Add tests for onCompositionStart and onCompositionUpdte events

The BeforeInputEventPlugin is responsible for emitting these events so we need to add tests for this. This also ensure we exercise the code path that, L207, that was not previously exercised with the public tests.
rhagigi pushed a commit to rhagigi/react that referenced this pull request Apr 19, 2018
…ackCompositionState (facebook#11849)

* Add test exercising public API to test BeforeInputEventPlugin + FallbackCompositionState

 - I've adopted a similar approach to the existing test for BeforeInputEventPlugin
 - I've simulated events and then assert the event handler for onBeforeInput is fired or not fired based on the test conditions
 - The scenarios are tested against IE11, Webkite and Presto environment simulations
 - I've encorporated what I understand to be the functionality in the FallbackCompositionState test

* Prettier

* Linting fixes

* Remove test for contenteditable in Presto - the contenteditable type is not supported in Presto powered browsers (Opera).

* Remove mention of Presto as this explicit condition is no longer handled in BeforeInputEventPlugin.

We still need to exercise usage of FallbackCompositionState though so let's keep a test where the env does not support Composition and Text events.

* Add tests for envs with only CompositionEvent support

* Remove internal tests no longer needed

* Shorten test case names to satisfy lint rules

* Add tests for onCompositionStart and onCompositionUpdte events

The BeforeInputEventPlugin is responsible for emitting these events so we need to add tests for this. This also ensure we exercise the code path that, L207, that was not previously exercised with the public tests.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants