Multi selection - implements #22 #23

geowarin · 2015-09-25T09:01:01Z

Implements #22.

I've added tests for every use case detailed in the issue.
There are a couple of things that can be improved immediately :

Since we have the element() method, I think we can safely remove the params methods in tests.js which becomes somewhat obsolete. This will allow to remove the ugly unarray method
The find middleware has become a bit more complicated but I think that's ok. Anyway, we can find much better names for the functions declared in there

I'm looking forward for your reviews and comments :)

This makes the API more uniform. Previously, elements could be either an array or a single element which forces middleware authors to handle both cases. For the users, when calling the test() method, helpers.elements will always contain arrays. However, they can still benefit from the shortcut syntax test({myElement}). In that case, elements will be 'unnaraid', which means that that they can either get an array of elements or a single element depending on their component layout.

geowarin · 2015-09-25T09:56:12Z

I also added two middlewares:

The Index middleware

Allows to select a specific element after a multi find

Test(<TableComponent/>)
  .find('table', {multi: false})
  .find('td', {multi: true})
  .index(1)
  .element(td => {
    expect(td.getDOMNode().textContent).to.equal('2')
  })

The setValues middleware

Test(<FormComponent />)
  .find('input', {multi: true})
  .setValues(['username', 'password'])

This middleware allows to select a single element after a multi find

zackify · 2015-09-25T17:15:42Z

I'll look this over tonight!

zackify · 2015-09-26T01:59:51Z

src/tests.js

-import {Find, SetState, Simulate} from './middleware'
+import {Find, SetState, Simulate, SetValues, Index} from './middleware'
+
+const unarray = (elems) => {


Since we're making breaking changes, might as well remove this like you said.

zackify · 2015-09-26T02:02:30Z

So it all looks good to me, just remove the params method, update the readme, add a list of the breaking changes, and I'll merge it unless if @dphaener or @kenwheeler see anything wrong. We need to start a wiki with all of the options and examples

vramana · 2015-09-26T02:05:47Z

src/middleware/find.js

@@ -1,18 +1,62 @@
-import React from 'react'
-let { TestUtils } = React.addons
+import React from 'react';


Why add semicolons here?? May be you can add no-semicolon rule to the eslintrc. So, the style is consistent across the codebase.

Agreed, call me evil, but let's remove them all.

…nt rule

vramana · 2015-09-26T09:02:22Z

tests/components/component.jsx

@@ -1,13 +1,13 @@
-import React from 'react'
+import {Component} from 'react'


You need to do import React, { Component } from 'react'

Why? The React object is not used in those components

You are. All the JSX components you write are converted to React.createElement(tagName, ...) Babel transpiles this but it doesn't import it.

…sx test

geowarin · 2015-09-26T09:57:48Z

src/middleware/find.js

+  const {elements, name} = findSingleOrMulti(rootElement, selector)
+
+  if (elements.length === 0)
+    throw new Error(`Could not find element "${selector}"`)


So there was a debate about this throw between me and @vramana.
I think we should throw early to improve user feedback and be able to write a test to assert this behavior.

On the other hand if we don't throw, elements[name] will be undefined somewhere later.
@zackify, thought?

Throwing an error there is fine with me

This can be useful to simulate a change event on a bunch of inputs for instance

Rename the 'tests' folder to 'test', which is more standard and moved mocha.opts in there Make the tests fall in two folders: core and middlewares Make eslint a post-test script.

zackify · 2015-09-26T21:32:34Z

README.md

+  })
+~~~
+
+But if you use the `multi` option, you can find nested elements:


You should make multi the default, like you said. Also, I don't think I like the root option. The component instance is always available inside the test method if you need it

geowarin · 2015-09-27T16:24:28Z

Ok guys, the final API is like this:

Test(<FormComponent login={spy}/>)
  .find('input').setValues(['username', 'password']) // multi by default
  .and() // come back to the root
  .find('form').first().simulate('submit')
  .test(() => {
    expect(spy.calledWith('username', 'password')).to.be.true;
  })

You can now use the first(), last() and index() middleware if you want to work on a specific element.

What do you think?

vramana · 2015-09-28T02:17:03Z

In your example, lets say the sumbit returns error and we want to test with new values for username and password. Then I have to do find('input'), find('form'), simulate('submit') all over again. But we already have references for these elements, Why do we have to find them?

One way to avoid this is to return is to check if the element is present in elements. But this doesn't allow form element to be over overridden by some other form inside the component. I feel that we are testing a component with lots of elements it is slightly hard to this via this api.

For finding nested elements, isn't it simpler to write a query like this find('table td') Then the immediate question is what will be the name of it in elements. May be we can pass an extra argument specifying name of the element i.e, find('cells', 'table td').

Are there any generic cases where test method is useful ?? We can do most of things by element method. I feel it should be the way to go because having one unambiguous way to write tests helps.

vramana · 2015-09-28T02:20:01Z

src/middleware/and.js

@@ -0,0 +1,3 @@
+export default function And() {
+  this.root = this.instance


This should be a method inside Test class because it doesn't fit the definition of a middleware.

geowarin · 2015-09-28T08:03:19Z

@vramana I like your suggestions, especially the possibility to name the elements that you find.
However, this does not allow naming only the first button for instance if I have a bunch of them.

Right now, the test() method is useful to assert things on the component like its props or state.
But we could create an instance() method that injects the component instance.

geowarin · 2015-09-28T08:12:12Z

I also fear that leaving the possibility for the user to find something complicated like find('table .nested td') will make them want to do find('table > td.nested') and we will soon be reinventing jquery selectors.

vramana · 2015-09-28T14:39:21Z

@geowarin I don't think we need to reinvent them. Even I was thinking this for a while. I think I found a solution. Once we have the DOM for table we can use querySelectorAll on it to filter of elements. I think it will work in most cases. May be I am missing something. I'll think more about this.

geowarin · 2015-09-28T15:08:13Z

That would be awesome, except you cannot call ReactUtils.simulate() on a DOM node, can you?

vramana · 2015-09-28T15:14:50Z

Yeah. This is going to be hard I guess. Let me think what can be done.

geowarin · 2015-09-29T08:14:40Z

This has become a messy PR. I'm closing this waiting for #24 to rebase upon.
I invite @vramana, @zackify and any person who is interested in proposing an API for nested searches to continue the discussion here: #22.

Of course, I am volunteering for a new PR 😄

geowarin added 11 commits September 25, 2015 10:44

Add error messages

3e87072

Add setValues middleware

49e6a93

Refactor find elements

f2778f8

write a failing tests

4dd1589

Simplify simulate middleware

fdc223c

write failing test for multi find

6994606

rebase upstream

6b36b87

Find deeply nested elements

318aab9

Rebased on upstream master

b0217a4

Fixed gitignore

80a265d

Add the index middleware

aea3414

This middleware allows to select a single element after a multi find

zackify reviewed Sep 26, 2015
View reviewed changes

vramana reviewed Sep 26, 2015
View reviewed changes

geowarin added 3 commits September 26, 2015 09:20

Remove special case for params and enforce no semicolons with an esli…

3d89d83

…nt rule

Enforce the usage of const with an eslint rule

f797f18

Add .editorconfig to npmignore

e91c753

geowarin added 2 commits September 26, 2015 10:52

Improve code style

8beff83

Make the middleware parameters more explicit

ff2ffa7

vramana reviewed Sep 26, 2015
View reviewed changes

geowarin added 4 commits September 26, 2015 11:28

import React in components and use const in tests. Rename the index.j…

63ee221

…sx test

Update the README to remove references to the helpers array

a3dd9f8

Use const instead of var

c69eb94

Remove unnecessary code

0000a85

geowarin reviewed Sep 26, 2015
View reviewed changes

geowarin added 3 commits September 26, 2015 14:30

Remove unused method

3493e1b

Let simulate operate on an array of elements

c28c39e

This can be useful to simulate a change event on a bunch of inputs for instance

Refactor the test folder

e2283ba

Rename the 'tests' folder to 'test', which is more standard and moved mocha.opts in there Make the tests fall in two folders: core and middlewares Make eslint a post-test script.

zackify reviewed Sep 26, 2015
View reviewed changes

geowarin added 3 commits September 27, 2015 17:53

Add First, last and And middlewares

b1acd58

Make find middleware always multi

571c1e8

Update the README to reflect API changes

f4bdd48

vramana reviewed Sep 28, 2015
View reviewed changes

Make the 'and' middleware a method in Test

b01b3f8

geowarin mentioned this pull request Sep 28, 2015

[WIP] Refine API #24

Merged

7 tasks

geowarin closed this Sep 29, 2015

geowarin mentioned this pull request Sep 30, 2015

Improve frontend tests geowarin/boot-react#8

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi selection - implements #22 #23

Multi selection - implements #22 #23

geowarin commented Sep 25, 2015

geowarin commented Sep 25, 2015

zackify commented Sep 25, 2015

zackify Sep 26, 2015

zackify commented Sep 26, 2015

vramana Sep 26, 2015

zackify Sep 26, 2015

vramana Sep 26, 2015

geowarin Sep 26, 2015

vramana Sep 26, 2015

geowarin Sep 26, 2015

zackify Sep 26, 2015

zackify Sep 26, 2015

geowarin commented Sep 27, 2015

vramana commented Sep 28, 2015

vramana Sep 28, 2015

geowarin commented Sep 28, 2015

geowarin commented Sep 28, 2015

vramana commented Sep 28, 2015

geowarin commented Sep 28, 2015

vramana commented Sep 28, 2015

geowarin commented Sep 29, 2015

		@@ -1,13 +1,13 @@
		import React from 'react'
		import {Component} from 'react'

		@@ -0,0 +1,3 @@
		export default function And() {
		this.root = this.instance

Multi selection - implements #22 #23

Multi selection - implements #22 #23

Conversation

geowarin commented Sep 25, 2015

geowarin commented Sep 25, 2015

The Index middleware

The setValues middleware

zackify commented Sep 25, 2015

Choose a reason for hiding this comment

zackify commented Sep 26, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geowarin commented Sep 27, 2015

vramana commented Sep 28, 2015

Choose a reason for hiding this comment

geowarin commented Sep 28, 2015

geowarin commented Sep 28, 2015

vramana commented Sep 28, 2015

geowarin commented Sep 28, 2015

vramana commented Sep 28, 2015

geowarin commented Sep 29, 2015