Test Thoughts & Subthoughts #312

mekanix · 2020-01-30T21:57:23Z

Refers to #244

mekanix · 2020-01-31T13:47:15Z

Only first test is passing, no matter which test it is. I'm suspecting I need to clean up something or reinit firebase or something, so I'm closing this PR for now.

mekanix · 2020-01-31T14:57:47Z

Everything works if we have one it section per test file. As test file naming scheme doesn't have to follow component naming, we can have as many test files as we want, so I think it's an acceptable trade-off.

raineorshine · 2020-02-03T18:56:31Z

I am unfortunately getting the following error when I try to run the tests:

> [email protected] test /Users/raine/projects/em
> react-scripts test --env=jsdom --silent

2020-02-03 11:54 node[66157] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22)
2020-02-03 11:54 node[66157] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22)
2020-02-03 11:54 node[66157] (FSEvents.framework) FSEventStreamStart: register_with_server: ERROR: f2d_register_rpc() => (null) (-22)
events.js:187
      throw er; // Unhandled 'error' event
      ^

Error: EMFILE: too many open files, watch
    at FSEvent.FSWatcher._handle.onchange (internal/fs/watchers.js:123:28)
Emitted 'error' event on FSWatcher instance at:
    at FSEvent.FSWatcher._handle.onchange (internal/fs/watchers.js:129:12) {
  errno: -24,
  syscall: 'watch',
  code: 'EMFILE',
  filename: null
}
npm ERR! Test failed.  See above for more details.

raineorshine · 2020-02-03T18:57:36Z

Looking into a solution, as it may be OSX specific.

facebook/create-react-app#4540

raineorshine · 2020-02-03T19:07:34Z

Sweet, installing watchman was all it took to fix that.

raineorshine

I still have not figured out how to run more than one it block in the same file. It seems to not be properly cleaning up/unmounting, because the wrappers are returning nodes from the previous it. It's good that it is not cleaned up automatically, as we want the same component to be used across multiple it blocks when we are testing various properties without having to recreate the DOM each time, but I am not sure why unmount is not working. It would be nice to be able to reset for each describe block in some cases.

I have yet to test this potential solution out: enzymejs/enzyme#911

Another approach: uyhcire/tabliner@3e4c7d3

For now, we can do each full test scenario in a separate file.

src/util/initEvents.js

src/components/__tests__/EmptyThought.js

package.json

mekanix · 2020-02-04T13:10:50Z

Sweet, installing watchman was all it took to fix that.

Do you want me to add it as dev dependency?

raineorshine · 2020-02-04T17:36:46Z

Do you want me to add it as dev dependency?

No, too heavy. Developers can install it if needed for now.

mekanix · 2020-02-06T10:10:39Z

I left --env=jest in the package.json as without it there is no window or document objects, although there's a section in the .eslintrc.json that clearly states:

  "env": {
    "browser": true,
    "es6": true,
    "jest": true,
    "mocha": true
  },

src/components/App.js

raineorshine · 2020-02-06T21:27:34Z

src/components/__tests__/EmptyThought.js

+  wrapper = null
+})
+
+it('create and edit empty thought', async () => {


Create and edit should be separate tests.

Think about it, it's useless. In order to edit the thought, you need to create it, so why not test it? Same for delete.

I disagree. If there is one test for creating a thought and for editing a thought, and the test fails after some changes are made in the future, we won't know whether there was a regression in creating a thought or editing a thought. While editing a thought will never work if creating a thought is broken, the inverse is not true. Having separate tests isolates these test cases and prevents this ambiguity from arising.

But failing test tells you exact line where it broke, so we will know what exactly failed. If you still want me to have create and edit, no problem.

Good point. Either one is a reasonable approach. Let's go with separate tests since that is my preference.

src/components/__tests__/EmptyThought.js

src/components/__tests__/SubThought.js

raineorshine

Looking good, thanks!

Could you find a way to get rid of the render warning that appears in the tests?

Warning: render(): Rendering components directly into document.body is discouraged, since its children are often manipulated by third-party scripts and browser extensions. This may lead to subtle reconciliation issues. Try rendering into a container element created for your app.

I will fix the Failed prop type warning.

We should investigate why restoreSelection is failing as well.

raineorshine · 2020-02-08T15:25:01Z

src/setupTests.js

+    const skipTutorial = wrapper.find('div.modal-actions div a')
+    skipTutorial.simulate('click')
+    const keyboardResponder = wrapper.find('#keyboard')
+    await keyboardResponder.simulate('keydown', { key: 'Enter' })


Can we put the Enter event in the tests? I'm okay with the skipTutorial click happening here, but the new thought should go in each test. If a regression is introduced to creating a new thought, we want the newThought test to fail, not setupTests.

Still waiting on this

raineorshine · 2020-02-08T15:25:38Z

src/setupTests.js

+  }
+
+  await act(async () => {
+    // eslint-disable-next-line no-undef


I believe you can remove the eslint-disable lines related to jest now that we have jest specified as an eslint env.

raineorshine · 2020-02-08T15:38:11Z

src/components/__tests__/ThoughtCreate.js

@@ -0,0 +1,6 @@
+it('create thought', async () => {
+  const thought = document.wrapper.find(
+    'li.leaf div.thought-container div.thought div.editable',


Based on my tests, document.wrapper.find('div.editable') works fine.

.editable does not work because there is both a <ContentEditable className='editable'> and a <div className='editable'>

src/components/__tests__/ThoughtDelete.js

raineorshine · 2020-02-08T15:41:37Z

src/components/__tests__/SubThoughtCreate.js

+  await thought.update()
+  expect(thought.text()).toBe('c')
+  // eslint-disable-next-line fp/no-let
+  const subthoughtsData = getThoughtsRanked(['c'])


These variables are short enough that they can be inlined in the expect.

raineorshine · 2020-02-08T15:42:32Z

src/components/__tests__/SubThoughtDelete.js

+  await thought.update()
+  expect(thought.text()).toBe('c')
+  // eslint-disable-next-line fp/no-let
+  let subthoughtsData = getThoughtsRanked(['c'])


Let's avoid let here, and simply inline these short statements as needed.

raineorshine · 2020-02-08T15:42:39Z

src/components/__tests__/SubThoughtEdit.js

+  jest.runAllTimers()
+  await thought.update()
+  expect(thought.text()).toBe('c')
+  // eslint-disable-next-line fp/no-let


raineorshine · 2020-02-08T15:42:51Z

src/components/__tests__/SubThoughtTop.js

+  jest.runAllTimers()
+  await thought.update()
+  expect(thought.text()).toBe('c')
+  // eslint-disable-next-line fp/no-let


raineorshine · 2020-02-09T18:33:37Z

Also let's move the __tests__ folder up a level since technically we are testing more than components.

mekanix · 2020-02-10T10:40:17Z

I tried moving tests one directory up, but this is what I end up with:

> [email protected] lint /usr/src/em
> eslint src


> [email protected] test /usr/src/em
> react-scripts test --env=jsdom

/usr/src/em/node_modules/react-scripts/scripts/test.js:20
  throw err;
  ^

Error: The `document` global was defined when React was initialized, but is not defined anymore. This can happen in a test environment if a component schedules an update from an asynchronous callback, but the test has already finished running. To solve this, you can either unmount the component at the end of your test (and ensure that any asynchronous operations get canceled in `componentWillUnmount`), or you can change the test itself to be asynchronous.
    at Object.invokeGuardedCallbackDev (/usr/src/em/node_modules/react-dom/cjs/react-dom.development.js:298:17)
    at invokeGuardedCallback (/usr/src/em/node_modules/react-dom/cjs/react-dom.development.js:440:31)
    at commitRootImpl (/usr/src/em/node_modules/react-dom/cjs/react-dom.development.js:25020:9)
    at unstable_runWithPriority (/usr/src/em/node_modules/scheduler/cjs/scheduler.development.js:697:12)
    at runWithPriority$2 (/usr/src/em/node_modules/react-dom/cjs/react-dom.development.js:12149:10)
    at commitRoot (/usr/src/em/node_modules/react-dom/cjs/react-dom.development.js:24922:3)
    at finishSyncRender (/usr/src/em/node_modules/react-dom/cjs/react-dom.development.js:24329:3)
    at performSyncWorkOnRoot (/usr/src/em/node_modules/react-dom/cjs/react-dom.development.js:24307:9)
    at /usr/src/em/node_modules/react-dom/cjs/react-dom.development.js:12199:24
    at unstable_runWithPriority (/usr/src/em/node_modules/scheduler/cjs/scheduler.development.js:697:12)
    at runWithPriority$2 (/usr/src/em/node_modules/react-dom/cjs/react-dom.development.js:12149:10)
    at flushSyncCallbackQueueImpl (/usr/src/em/node_modules/react-dom/cjs/react-dom.development.js:12194:7)
    at flushSyncCallbackQueue (/usr/src/em/node_modules/react-dom/cjs/react-dom.development.js:12182:3)
    at batchedUpdates$1 (/usr/src/em/node_modules/react-dom/cjs/react-dom.development.js:24392:7)
    at Object.notify (/usr/src/em/node_modules/react-redux/lib/utils/Subscription.js:29:7)
    at Subscription.notifyNestedSubs (/usr/src/em/node_modules/react-redux/lib/utils/Subscription.js:71:20)
    at Subscription.handleChangeWrapper (/usr/src/em/node_modules/react-redux/lib/utils/Subscription.js:76:12)
    at dispatch (/usr/src/em/node_modules/redux/lib/redux.js:228:7)
    at Object.dispatch (/usr/src/em/node_modules/redux-thunk/lib/index.js:14:16)
    at /usr/src/em/src/util/loadLocalState.js:111:18
    at processTicksAndRejections (internal/process/task_queues.js:97:5)
npm ERR! Test failed.  See above for more details.

raineorshine · 2020-02-11T20:58:22Z

I tried moving tests one directory up, but this is what I end up with

Okay, that's too bad.

raineorshine · 2020-02-11T21:03:02Z

Thank you, those are perfectly sized and named commits.

raineorshine · 2020-02-20T20:26:54Z

Well, after a couple hours of trying different things myself, I was also unable to get the Editable element to update in the unit test. None of the tips in enzymejs/enzyme#76 seemed to work.

That said, the change event works great and the store is updated, so we can call getThoughts which accesses the store directly to see the changed value. Will you utilize this approach in all unit tests?

import { ROOT_TOKEN } from '../../constants.js'
import { getThoughts } from '../../util.js'

it('edit thought', async () => {
  const thought = document.wrapper.find('div.editable')
  expect(thought.text()).toBe('')

  await thought.simulate('change', { target: { value: 'c' } })
  jest.runAllTimers()

  // for some reason the text of the Editable element is not updated
  // await document.wrapper.update()
  // expect(document.wrapper.find('div.editable').text()).toBe('c')

  const children = getThoughts([ROOT_TOKEN])
  expect(children).toHaveLength(1)
  expect(children[0]).toMatchObject({ value: 'c', rank: 0 })
})

raineorshine · 2020-02-20T20:44:40Z

These changes are important to set EDIT_THROTTLE = 0 during testing and fix the auto merge:

766e1a6

src/components/__tests__/SubThoughtCreate.js

src/components/__tests__/SubThoughtDelete.js

src/components/__tests__/SubThoughtEdit.js

src/components/__tests__/SubThoughtTop.js

src/components/__tests__/ThoughtDelete.js

raineorshine · 2020-02-21T18:33:24Z

src/setupTests.js

+
+  await act(async () => {
+    jest.useFakeTimers()
+    const originalError = console.error


Hey, I'm not really comfortable with disabling console.error to hide the error. It seems like it's just a workaround. Let's try to get to the source of the problem. If it is impossible to remove, it deserves a clear comment explaining why.

I think https://github.com/facebook/react/blob/master/packages/react-dom/src/client/ReactDOMLegacy.js#L81 is where we get that error from. As I don't see any way to go around it except creating new console.error which filters what it outputs by content (we can output only if first argument to console.error is not the one from the link).

New console.error mock is more precise in what it silences. Good enough?

Mounting to a child of document.body will fix the error without the workaround:

// mount to child to avoid warning about rendering directly to document.body const root = document.body.appendChild(document.createElement('div')) const wrapper = await mount( <div id="keyboard" onKeyDown={keyDown} tabIndex="0" > {app} </div>, { attachTo: root }, )

raineorshine · 2020-02-22T18:32:56Z

I was going to try this solution, but this branch is crashing as-is when I run npm run test. Is it working for you?

  ● Console

    console.warn src/reducers/settings.js:25
      Missing oldThoughtRanked in Settings update: Tutorial Off


 RUNS  src/components/__tests__/SubThoughtCreate.js
/Users/raine/projects/em/node_modules/react-scripts/scripts/test.js:20
  throw err;
  ^

TypeError: Cannot read property '_location' of null
    at Window.get location [as location] (/Users/raine/projects/em/node_modules/jsdom/lib/jsdom/browser/Window.js:148:79)
    at Object.<anonymous>.exports.loadLocalState (/Users/raine/projects/em/src/util/loadLocalState.js:56:32)
npm ERR! Test failed.  See above for more details.

mekanix · 2020-02-22T22:02:40Z

I do see it in the log, but it doesn't break. This is the whole log on my machine http://dpaste.com/27JD3PE

raineorshine · 2020-02-24T16:15:30Z

Great! The unmount fixed it. All tests passing for me.

src/components/__tests__/SubThoughtCreate.js

src/components/__tests__/SubThoughtEdit.js

src/components/__tests__/SubThoughtTop.js

src/components/__tests__/ThoughtDelete.js

src/components/__tests__/ThoughtAbove.js

src/components/__tests__/SubThoughtCreate.js

raineorshine · 2020-02-24T17:53:11Z

src/setupTests.js

+    const skipTutorial = wrapper.find('div.modal-actions div a')
+    skipTutorial.simulate('click')
+    const keyboardResponder = wrapper.find('#keyboard')
+    await keyboardResponder.simulate('keydown', { key: 'Enter' })


Still waiting on this

raineorshine · 2020-02-24T17:57:30Z

src/setupTests.js

+
+  await act(async () => {
+    jest.useFakeTimers()
+    const originalError = console.error


Mounting to a child of document.body will fix the error without the workaround:

// mount to child to avoid warning about rendering directly to document.body const root = document.body.appendChild(document.createElement('div')) const wrapper = await mount( <div id="keyboard" onKeyDown={keyDown} tabIndex="0" > {app} </div>, { attachTo: root }, )

mekanix · 2020-02-25T16:00:05Z

Github doesn't let me reply on last two comments.

Still waiting on this:
Isn't it more logical to have something in setup that will happen every single time? If not, I'll copy/paste it into tests, but it makes no sense to me.

document.body is not used any more

raineorshine · 2020-02-25T17:37:35Z

Isn't it more logical to have something in setup that will happen every single time? If not, I'll copy/paste it into tests, but it makes no sense to me.

That's a fair argument I suppose. To me it seems clear that creating a thought is a testable action, not part of setup. It is also more future-proof, as there will likely be tests that do not begin by creating a new thought.

mekanix closed this Jan 31, 2020

mekanix reopened this Jan 31, 2020

raineorshine requested changes Feb 3, 2020

View reviewed changes

src/util/initEvents.js Outdated Show resolved Hide resolved

src/components/__tests__/EmptyThought.js Outdated Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

mekanix force-pushed the feature/test-thought branch from 35fad43 to 8d80593 Compare February 4, 2020 13:25

mekanix force-pushed the feature/test-thought branch from 2e66407 to 88cfaca Compare February 6, 2020 09:48

raineorshine requested changes Feb 6, 2020

View reviewed changes

mekanix force-pushed the feature/test-thought branch from a80dfe4 to dd15d4b Compare February 7, 2020 16:10

raineorshine requested changes Feb 8, 2020

View reviewed changes

mekanix force-pushed the feature/test-thought branch from 071f35e to 0e7f5db Compare February 8, 2020 22:06

mekanix force-pushed the feature/test-thought branch from c6e3dd5 to 700d892 Compare February 11, 2020 09:18

mekanix force-pushed the feature/test-thought branch from 700d892 to facb4d6 Compare February 19, 2020 16:25

mekanix force-pushed the feature/test-thought branch from e1484cc to 12f5dd4 Compare February 21, 2020 10:27

raineorshine requested changes Feb 21, 2020

View reviewed changes

mekanix force-pushed the feature/test-thought branch 2 times, most recently from e61fe08 to ccf085f Compare February 21, 2020 20:05

mekanix force-pushed the feature/test-thought branch from d5f8716 to 2d1b58f Compare February 24, 2020 09:49

raineorshine requested changes Feb 24, 2020

View reviewed changes

Test thought

b0be19c

mekanix force-pushed the feature/test-thought branch from 86dc39d to b0be19c Compare February 26, 2020 10:02

This was referenced Mar 1, 2020

New lint settings and typescript #381

Closed

Test Thoughts & Subthoughts (raineorshine) #392

Merged

raineorshine changed the title ~~Test thought~~ Test Thoughts & Subthoughts Mar 1, 2020

raineorshine merged commit 40d8724 into cybersemics:dev Mar 1, 2020

Test Thoughts & Subthoughts #312

Test Thoughts & Subthoughts #312

Conversation

mekanix commented Jan 30, 2020 • edited Loading

mekanix commented Jan 31, 2020

mekanix commented Jan 31, 2020

raineorshine commented Feb 3, 2020

raineorshine commented Feb 3, 2020

raineorshine commented Feb 3, 2020

raineorshine left a comment • edited Loading

Choose a reason for hiding this comment

mekanix commented Feb 4, 2020

raineorshine commented Feb 4, 2020

mekanix commented Feb 6, 2020

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

raineorshine left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raineorshine commented Feb 9, 2020

mekanix commented Feb 10, 2020

raineorshine commented Feb 11, 2020

raineorshine commented Feb 11, 2020

raineorshine commented Feb 20, 2020

raineorshine commented Feb 20, 2020

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

raineorshine commented Feb 22, 2020

mekanix commented Feb 22, 2020

raineorshine commented Feb 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mekanix commented Feb 25, 2020

raineorshine commented Feb 25, 2020

mekanix commented Jan 30, 2020 •

edited

Loading

raineorshine left a comment •

edited

Loading