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

Test store data types update & fixes #427

Merged
merged 26 commits into from Jan 8, 2019

Conversation

Projects
None yet
2 participants
@T4rk1n
Copy link
Contributor

commented Dec 28, 2018

Closes #422

@T4rk1n T4rk1n changed the title [WIP] Test store datatypes & fixes [WIP] Test store data types update & fixes Dec 28, 2018

* @param {any} oldData - The old data to compare
* @returns {boolean} The data has changed.
*/
function dataChanged(newData, oldData) {

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Dec 28, 2018

Contributor

Beautiful, thank you!

const oldNull = R.isNil(old);
const newNull = R.isNil(data);
const oldNull = R.isNil(oldData);
const newNull = R.isNil(newData);

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Dec 28, 2018

Contributor

isNil catches both undefined and null - and mostly that distinction is moot because undefined can't be sent via JSON, I guess. But we should be careful about it in objects, like {a: null} <-> {}

for (let i = 0; i < data.length; i++) {
if (dataCheck(data[i], old[i])) {
for (let i = 0; i < newData.length; i++) {
if (dataChanged(newData[i], oldData[i])) {
return true;
}
}
} else if (R.contains(type, ['String', 'Number'])) {

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Dec 28, 2018

Contributor

'Boolean'?

This comment has been minimized.

Copy link
@T4rk1n

T4rk1n Dec 28, 2018

Author Contributor

Forgot about Boolean in all this type checks, thanks!

@T4rk1n

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2018

Look like removing the falsy check in componentDidUpdate fixed the basic types changes. 🎉

I added a type check:

if (type !== R.type(oldData)) {
    return true;
}

But it failed the test.

Added Boolean to valid types.

I'll add more edge cases test later.

@alexcjohnson

This comment has been minimized.

Copy link
Contributor

commented Dec 28, 2018

I added a type check ... But it failed the test.

Can you say more? I don't see how that makes sense and I worry about remaining strange cases like #422 (comment) if this is omitted.

@T4rk1n

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2018

Can you say more?

Failure: AssertionError: '1' != '{"data": [1, 2, 3]}'

@T4rk1n T4rk1n force-pushed the test-store-datatypes branch from 9a3c0f5 to 0c9dfe4 Dec 28, 2018

self.assertEqual(
assertion_text,
self.driver.find_element_by_css_selector(selector).text
text_equal

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Dec 28, 2018

Contributor

Ah, much nicer 🏆

In case anyone else hits this issue in the future: EC.text_to_be_present_in_element does a substring match, not a full value match. Makes sense in retrospect, given its name... so if the previous value is a substring of the new value you're expecting in the element you can get a premature match 💥

@T4rk1n T4rk1n force-pushed the test-store-datatypes branch from 6fb2e1e to 7e276fe Jan 7, 2019

@T4rk1n T4rk1n changed the title [WIP] Test store data types update & fixes Test store data types update & fixes Jan 7, 2019

@T4rk1n

This comment has been minimized.

Copy link
Contributor Author

commented Jan 7, 2019

@alexcjohnson Please review.

('bool', False),
('empty-dict', {}),
('list-dict-1', [1, 2, {'data': [55, 66, 77], 'dummy': 'dum'}]),
('list-dict-1', [1, 2, {'data': [111, 99, 88]}]),

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Jan 8, 2019

Contributor

Can you add the pathological items in my comments #422 (comment) and #422 (comment)? I think you fixed all those, but here's another one to add, that I think still fails:

dataChanged({a: 1, b: null}, {a: 1, c: 1}) // returns false, should be true

This is due to isNil conflating undefined and null - come to think of it, is there really a reason to treat those as equivalent? Perhaps we can just change the null check to:

if (oldNull || newNull) {
    return oldData !== newData; // which will distinguish null/undefined
}
@T4rk1n

This comment has been minimized.

Copy link
Contributor Author

commented Jan 8, 2019

@alexcjohnson Added the undef edge case and checking the data instead of the null fixed it. I think we're good now.

@alexcjohnson
Copy link
Contributor

left a comment

Excellent, thanks for the changes. 💃

@T4rk1n T4rk1n merged commit 52b804d into master Jan 8, 2019

4 checks passed

ci/circleci: python-2.7 Your tests passed on CircleCI!
Details
ci/circleci: python-3.6 Your tests passed on CircleCI!
Details
ci/circleci: python-3.7 Your tests passed on CircleCI!
Details
percy/dash-core-components No visual changes since last approval
Details

@T4rk1n T4rk1n deleted the test-store-datatypes branch Jan 8, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.