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.

Sign up for GitHub

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

Integrate with a CSS parser for selector parsing #1086

Merged
merged 25 commits into from
Sep 16, 2017
Merged

Integrate with a CSS parser for selector parsing #1086

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
8177e26
Implement new selector parsing module
Aug 21, 2017
0d4a7eb
Use new selector API in mount and shallow wrappers
Aug 21, 2017
51ff0e7
Implement findParentNode utility
Aug 21, 2017
26bf200
Rename selector tests file
Aug 21, 2017
fa80c0e
Stop treating unquoted strings as invalid
Aug 21, 2017
b18cfe4
Update tests
Aug 21, 2017
5dde9c6
Add more comments to selectors.js
Aug 21, 2017
f20b0c4
Linting fixes
Aug 21, 2017
d7abfbe
Remove ComplexSelector
Aug 21, 2017
8bffb8a
Remove old predicate/selector parsing utilities
Aug 21, 2017
10019da
Remove invalid attribute test
Aug 21, 2017
5efaf08
Throw an error for complex selectors in buildPredicate
Aug 21, 2017
83c6f04
Move combinator logic into discrete utility functions
Aug 21, 2017
8b3e008
Replace scalpel with new rst-selector-parser package
Aug 22, 2017
1a73724
Throw for pseudo-class and psuedo-element selectors
Aug 23, 2017
36e10d2
Add more tests for selectors
Aug 23, 2017
ae733ee
Add test for URLS in attribute values
Aug 25, 2017
60b6a86
Improve test coverage, use correct APIs
Sep 1, 2017
2c3227b
Remove uuid from enzyme package.json
Sep 1, 2017
d0d810e
Fix error type in selector assertion
Sep 1, 2017
459108a
Fix react prop test assertion
Sep 1, 2017
188ed1c
Use if/else for branching in selector predicate functions
Sep 1, 2017
17f366d
Add nodeHasProperty test for NaN and Infinity
Sep 13, 2017
aadbc4a
Add test for adjacent siblings in arrays
Sep 13, 2017
a79e150
Use _reactInternalFiber for 16 adapater
Sep 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/enzyme-adapter-react-16/src/ReactSixteenAdapter.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,7 @@ class ReactSixteenAdapter extends EnzymeAdapter {
instance = null;
},
getNode() {
return instance ? toTree(instance._reactInternalInstance).rendered : null;
return instance ? toTree(instance._reactInternalFiber).rendered : null;
},
simulateEvent(node, event, mock) {
const mappedEvent = mapNativeEventNames(event);
Expand Down
193 changes: 0 additions & 193 deletions packages/enzyme-test-suite/test/ComplexSelector-spec.jsx
View file
Open in desktop

This file was deleted.

127 changes: 46 additions & 81 deletions packages/enzyme-test-suite/test/RSTTraversal-spec.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,9 +2,6 @@ import './_helpers/setupAdapters';
import React from 'react';
import sinon from 'sinon';
import { expect } from 'chai';
import {
splitSelector,
} from 'enzyme/build/Utils';
import { elementToTree } from 'enzyme-adapter-utils';
import {
hasClassName,
Expand All @@ -13,39 +10,13 @@ import {
treeFilter,
pathToNode,
getTextFromNode,
buildPredicate,
} from 'enzyme/build/RSTTraversal';
import { describeIf } from './_helpers';
import { REACT013 } from './_helpers/version';

const $ = elementToTree;

describe('RSTTraversal', () => {
describe('splitSelector', () => {
const fn = splitSelector;
it('splits multiple class names', () => {
expect(fn('.foo.bar')).to.eql(['.foo', '.bar']);
expect(fn('.foo.bar.baz')).to.eql(['.foo', '.bar', '.baz']);
});

it('splits tag names and class names', () => {
expect(fn('input.bar')).to.eql(['input', '.bar']);
expect(fn('div.bar.baz')).to.eql(['div', '.bar', '.baz']);
expect(fn('Foo.bar')).to.eql(['Foo', '.bar']);
});

it('splits tag names and attributes', () => {
expect(fn('input[type="text"]')).to.eql(['input', '[type="text"]']);
expect(
fn('div[title="title"][data-value="foo"]'),
).to.eql(['div', '[title="title"]', '[data-value="foo"]']);
});

it('throws for malformed selectors', () => {
expect(() => fn('div[data-name="xyz"')).to.throw(/Enzyme::Selector received what appears to be a malformed string selector/);
Copy link
Member

Choose a reason for hiding this comment

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

does this no longer throw (somewhere)? it's a malformed selector.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does throw, but since the new rst-selector-parser manages all parsing this should be covered by the unit tests for the package. I figure at this point we'd be testing the behavior of a dependency with its own tests.

But I'll add back a test that ensures that we catch the general parsing error and re-throw something more specific. Then I think it's safe to assume invalid selectors will throw, and if the rst-selector-parser coverage is missing a case we can add it there.

On that note, I'd be happy to include rst-selector-parser as part of this monorepo if we want to.

});
});

describe('hasClassName', () => {

it('should work for standalone classNames', () => {
Expand Down Expand Up @@ -82,13 +53,13 @@ describe('RSTTraversal', () => {
const node = $(<div onChange={noop} title="foo" />);

expect(nodeHasProperty(node, 'onChange')).to.equal(true);
expect(nodeHasProperty(node, 'title', '"foo"')).to.equal(true);
expect(nodeHasProperty(node, 'title', 'foo')).to.equal(true);
});

it('should not match on html attributes', () => {
const node = $(<div htmlFor="foo" />);

expect(nodeHasProperty(node, 'for', '"foo"')).to.equal(false);
expect(nodeHasProperty(node, 'for', 'foo')).to.equal(false);
});

it('should not find undefined properties', () => {
Expand All @@ -97,71 +68,73 @@ describe('RSTTraversal', () => {
expect(nodeHasProperty(node, 'title')).to.equal(false);
});

it('should parse false as a literal', () => {
const node = $(<div foo={false} />);

expect(nodeHasProperty(node, 'foo', 'false')).to.equal(true);
it('should parse booleans', () => {
expect(nodeHasProperty(<div foo />, 'foo', true)).to.equal(true);
expect(nodeHasProperty(<div foo />, 'foo', false)).to.equal(false);
expect(nodeHasProperty(<div foo />, 'foo', 'true')).to.equal(false);
expect(nodeHasProperty(<div foo={false} />, 'foo', false)).to.equal(true);
expect(nodeHasProperty(<div foo={false} />, 'foo', true)).to.equal(false);
expect(nodeHasProperty(<div foo={false} />, 'foo', 'false')).to.equal(false);
});

it('should parse false as a literal', () => {
const node = $(<div foo />);

expect(nodeHasProperty(node, 'foo', 'true')).to.equal(true);
Copy link
Member

Choose a reason for hiding this comment

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

should this test not still be valid for true instead of 'true'?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It should, I'll add that case.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added test case for booleans: https://github.com/airbnb/enzyme/pull/1086/files#diff-67b0df7735c607bdef49d8a20292e57aR71

});

it('should parse numbers as numeric literals', () => {
expect(nodeHasProperty(<div foo={2.3} />, 'foo', '2.3')).to.equal(true);
expect(nodeHasProperty(<div foo={2} />, 'foo', '2')).to.equal(true);
expect(() => nodeHasProperty(<div foo={2} />, 'foo', '2abc')).to.throw();
expect(() => nodeHasProperty(<div foo={2} />, 'foo', 'abc2')).to.throw();
expect(nodeHasProperty(<div foo={-2} />, 'foo', '-2')).to.equal(true);
expect(nodeHasProperty(<div foo={2e8} />, 'foo', '2e8')).to.equal(true);
expect(nodeHasProperty(<div foo={Infinity} />, 'foo', 'Infinity')).to.equal(true);
expect(nodeHasProperty(<div foo={-Infinity} />, 'foo', '-Infinity')).to.equal(true);
it('should parse numeric literals', () => {
expect(nodeHasProperty(<div foo={2.3} />, 'foo', 2.3)).to.equal(true);
expect(nodeHasProperty(<div foo={2} />, 'foo', 2)).to.equal(true);
expect(nodeHasProperty(<div foo={2} />, 'foo', '2abc')).to.equal(false);
expect(nodeHasProperty(<div foo={2} />, 'foo', 'abc2')).to.equal(false);
expect(nodeHasProperty(<div foo={-2} />, 'foo', -2)).to.equal(true);
expect(nodeHasProperty(<div foo={2e8} />, 'foo', 2e8)).to.equal(true);
expect(nodeHasProperty(<div foo={Infinity} />, 'foo', Infinity)).to.equal(true);
expect(nodeHasProperty(<div foo={-Infinity} />, 'foo', -Infinity)).to.equal(true);
});

it('should parse zeroes properly', () => {
expect(nodeHasProperty(<div foo={0} />, 'foo', '0')).to.equal(true);
expect(nodeHasProperty(<div foo={-0} />, 'foo', '-0')).to.equal(true);
expect(nodeHasProperty(<div foo={1} />, 'foo', '0')).to.equal(false);
expect(nodeHasProperty(<div foo={2} />, 'foo', '-0')).to.equal(false);
expect(nodeHasProperty(<div foo={0} />, 'foo', 0)).to.equal(true);
expect(nodeHasProperty(<div foo={0} />, 'foo', +0)).to.equal(true);
expect(nodeHasProperty(<div foo={-0} />, 'foo', -0)).to.equal(true);
Copy link
Member

Choose a reason for hiding this comment

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

can we add test cases that -0 does not match 0, and 0 does not match -0?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

expect(nodeHasProperty(<div foo={-0} />, 'foo', 0)).to.equal(false);
expect(nodeHasProperty(<div foo={0} />, 'foo', -0)).to.equal(false);
expect(nodeHasProperty(<div foo={1} />, 'foo', 0)).to.equal(false);
expect(nodeHasProperty(<div foo={2} />, 'foo', -0)).to.equal(false);
});

it('should work with empty strings', () => {
expect(nodeHasProperty(<div foo="" />, 'foo', '')).to.equal(true);
expect(nodeHasProperty(<div foo={''} />, 'foo', '')).to.equal(true);
expect(nodeHasProperty(<div foo={''} />, 'foo', '""')).to.equal(true);
expect(nodeHasProperty(<div foo={'bar'} />, 'foo', '')).to.equal(false);
expect(nodeHasProperty(<div foo={'bar'} />, 'foo', '""')).to.equal(false);
});

it('should work with NaN', () => {
expect(nodeHasProperty(<div foo={NaN} />, 'foo', 'NaN')).to.equal(true);
expect(nodeHasProperty(<div foo={0} />, 'foo', 'NaN')).to.equal(false);
expect(nodeHasProperty(<div foo={NaN} />, 'foo', NaN)).to.equal(true);
expect(nodeHasProperty(<div foo={0} />, 'foo', NaN)).to.equal(false);
});

it('should work with null', () => {
expect(nodeHasProperty(<div foo={null} />, 'foo', 'null')).to.equal(true);
expect(nodeHasProperty(<div foo={0} />, 'foo', 'null')).to.equal(false);
expect(nodeHasProperty(<div foo={null} />, 'foo', null)).to.equal(true);
expect(nodeHasProperty(<div foo={0} />, 'foo', null)).to.equal(false);
Copy link
Member

Choose a reason for hiding this comment

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

Let's also add a pair of assertions that null won't equal undefined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What's the expected behavior with undefined? It looks like it returns whether nodeProps has an own property matching the name, which means undefined ends up matching since foo exists.

Is that expected?

Copy link
Member

Choose a reason for hiding this comment

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

nodeHasProperty(<div foo={undefined} />, 'foo', undefined) should be true, nodeHasProperty(<div foo={undefined} />, 'foo', null) should be false, nodeHasProperty(<div foo={null} />, 'foo', undefined) should be false.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right now nodeHasProperty(<div foo={null} />, 'foo', undefined) returns true because it's defaulting to the own-property check. I think this behavior existed before this PR too, is that a bug? I'm not sure if that will break some existing tests.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ping @ljharb ^ I think that's the last point of clarification I need before this is good to go.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In the interest of time, i'm going to move forward w/ this PR. I've verified that this is the current behavior. If we would like to clarify it / change it in an upcoming PR, i'm fine with that. @aweary thanks for being so thorough here.

});

it('should work with false', () => {
expect(nodeHasProperty(<div foo={false} />, 'foo', 'false')).to.equal(true);
expect(nodeHasProperty(<div foo={0} />, 'foo', 'false')).to.equal(false);
expect(nodeHasProperty(<div foo={false} />, 'foo', false)).to.equal(true);
expect(nodeHasProperty(<div foo={0} />, 'foo', false)).to.equal(false);
});

it('should work with ±Infinity', () => {
expect(nodeHasProperty(<div foo={Infinity} />, 'foo', 'Infinity')).to.equal(true);
expect(nodeHasProperty(<div foo={0} />, 'foo', 'Infinity')).to.equal(false);
expect(nodeHasProperty(<div foo={-Infinity} />, 'foo', '-Infinity')).to.equal(true);
expect(nodeHasProperty(<div foo={0} />, 'foo', '-Infinity')).to.equal(false);
});

it('should throw when un unquoted string is passed in', () => {
const node = $(<div title="foo" />);

expect(() => nodeHasProperty(node, 'title', 'foo')).to.throw();
expect(nodeHasProperty(<div foo={Infinity} />, 'foo', Infinity)).to.equal(true);
expect(nodeHasProperty(<div foo={Infinity} />, 'foo', +Infinity)).to.equal(true);
Copy link
Member

Choose a reason for hiding this comment

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

let's add a case where Infinity fails to match a prop value of Infinity?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done.

Copy link
Member

Choose a reason for hiding this comment

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

It'd also be good to ensure NaN and Infinity don't equate, since both are not finite

expect(nodeHasProperty(<div foo={Infinity} />, 'foo', -Infinity)).to.equal(false);
expect(nodeHasProperty(<div foo={Infinity} />, 'foo', 'Infinity')).to.equal(false);
expect(nodeHasProperty(<div foo={Infinity} />, 'foo', NaN)).to.equal(false);
expect(nodeHasProperty(<div foo={0} />, 'foo', Infinity)).to.equal(false);
expect(nodeHasProperty(<div foo={-Infinity} />, 'foo', -Infinity)).to.equal(true);
expect(nodeHasProperty(<div foo={-Infinity} />, 'foo', Infinity)).to.equal(false);
expect(nodeHasProperty(<div foo={-Infinity} />, 'foo', Infinity)).to.equal(false);
expect(nodeHasProperty(<div foo={-Infinity} />, 'foo', '-Infinity')).to.equal(false);
expect(nodeHasProperty(<div foo={-Infinity} />, 'foo', NaN)).to.equal(false);
expect(nodeHasProperty(<div foo={NaN} />, 'foo', Infinity)).to.equal(false);
expect(nodeHasProperty(<div foo={NaN} />, 'foo', -Infinity)).to.equal(false);
expect(nodeHasProperty(<div foo={0} />, 'foo', -Infinity)).to.equal(false);
});

});

describe('treeForEach', () => {
Expand Down Expand Up @@ -367,12 +340,4 @@ describe('RSTTraversal', () => {
});
});
});

describe('buildPredicate', () => {
it('should throw expected error', () => {
const intSelector = 10;
const func = buildPredicate.bind(this, intSelector);
expect(func).to.throw(TypeError, 'Enzyme::Selector expects a string, object, or Component Constructor');
});
});
});
Loading