Skip to content
This repository has been archived by the owner on Jun 3, 2024. It is now read-only.

Add more Input tests, remove duplicate value callbacks. #384

Merged
merged 16 commits into from
Dec 4, 2018
Merged

Conversation

T4rk1n
Copy link
Contributor

@T4rk1n T4rk1n commented Nov 14, 2018

This PR adds a failing Input test and fix wait_for_text_to_equal that only checked if the text was present and not equal.

Tests are failing in dash and dash-renderer if we update the dash-core-components version to the latest.

@valentijnnieman
Copy link
Contributor

@T4rk1n The input has been changed to fire setProps() onBlur, causing these tests to fail - they're asserting call_count to be 2, when it is now 3, for example. We fire setProps onBlur now because we've added the debounce feature, allowing users to only fire setProps upon submit or onBlur, fixing the problems users had entering floats like 0.001, where setProps was being fired onChange caused setProps to truncate the 0.0 to just 0, overwriting the state of the input from 0.0 to 0. In other words, the tests need to be changed, or we only fire setProps(value) onBlur when debounce is true (that's probably the better solution).

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Nov 16, 2018

@valentijnnieman That is fine for the call count tests. But there's still the bug in #362, it's hard to reproduce by hand because you have to type and change focus very fast, but it can happen. The only way I had that test not fail is by removing the state.value from props.value and that would remove the debounce logic. Or by keeping the initial value and checking it's not the same, but that is not a good solution either.

@rmarren1
Copy link
Contributor

rmarren1 commented Nov 27, 2018

The input has been changed to fire setProps() onBlur, causing these tests to fail - they're asserting call_count to be 2, when it is now 3,

This is one of the errors I am experiencing in plotly/dash-renderer#100.

Is the other error also caused by this. It looks like the WebElement input is not properly cleared when input.clear() is called.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Nov 27, 2018

Both of those errors comes from that. The input get cleared ok, but it get the initial value back in componentWillReceiveProps caused by n_blur sending the value back or something when the input lose focus. It has to lose focus real fast when there is a value callback still going on to happen.

@rmarren1
Copy link
Contributor

okay, thanks!

@valentijnnieman
Copy link
Contributor

@T4rk1n Are you still working on this? Would be good to get this fixed!

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Nov 29, 2018

The only fix for that bug is removing the componentWillReceiveProps, it would remove the debounce logic for the number type. Maybe we can have another logic instead for that case. Like caching the value when the type is number and only setting it while setting n_blur/n_submit and only use the props for render.

@T4rk1n T4rk1n force-pushed the fix-tests branch 3 times, most recently from 3699717 to 967dd66 Compare December 3, 2018 23:03
@T4rk1n
Copy link
Contributor Author

T4rk1n commented Dec 4, 2018

After investigating a bit more the input clear bug, it boils down to the way selenium clear the input. It doesn't send the onChange event for the cleared value so somehow the initial value get backs in. This is not something that would happen if the user cleared the input manually with backspaces or selecting the whole text and deleting. Thus, it is safe to patch the failing tests to fix that selenium bug.

The callback counts were wrong because we added the debounce logic and the value was set all the time in onBlur and onSubmit. I added a check that only set the value if debounce is true, removing the duplicate value callbacks.

@T4rk1n
Copy link
Contributor Author

T4rk1n commented Dec 4, 2018

@valentijnnieman Please review

@T4rk1n T4rk1n changed the title [WIP] Add more Input tests. Add more Input tests, remove duplicate value callbacks. Dec 4, 2018
Copy link
Contributor

@valentijnnieman valentijnnieman left a comment

Choose a reason for hiding this comment

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

👍 Very happy these tests got fixed, thanks @T4rk1n! Got 2 small comments.

@@ -65,22 +65,27 @@ export default class Input extends Component {
if (setProps) {
const castValue =
type === 'number' ? Number(value) : value;
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency, the way the payload is set up should probably be the same in the onBlur method as well as in the onKeyPress method. I might prefer the way it's done on line 86 - the castvalue const also shouldn't have to be defined if it's not being used here.

@@ -1446,3 +1452,127 @@ def _insert_cookie(rep):
self.wait_for_text_to_equal('#content', 'Logged out')

self.assertFalse(self.driver.get_cookie('logout-cookie'))

def test_lose_focus_input(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this test really different from the test_simple_callback one? There, on line 1569, the input also loses focus. Perhaps this test could be removed?

Copy link
Contributor

@valentijnnieman valentijnnieman left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@T4rk1n T4rk1n merged commit 19ac349 into master Dec 4, 2018
@T4rk1n T4rk1n deleted the fix-tests branch December 4, 2018 20:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants