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

Upgrade to react-scripts v4 #919

Closed
raineorshine opened this issue Nov 21, 2020 · 6 comments · Fixed by #927
Closed

Upgrade to react-scripts v4 #919

raineorshine opened this issue Nov 21, 2020 · 6 comments · Fixed by #927
Assignees
Labels
test Testing and project configuration

Comments

@raineorshine
Copy link
Contributor

The pullQueue tests fail on react-scripts v4. It is related to the fake timer and mock debounce logic, but I can't figure out how to fix it. We should be able to use useFakeTimers('modern') now and not have to mock lodash's debounce, but that causes an error.

Here are the CHANGELOGS although I didn't find anything relevant:

@raineorshine raineorshine added the test Testing and project configuration label Nov 21, 2020
@shresthabijay shresthabijay self-assigned this Nov 24, 2020
@shresthabijay
Copy link
Collaborator

shresthabijay commented Nov 25, 2020

@raineorshine I finally figured out why useFakeTimers('modern') is not working. Even though we upgraded the react-scripts which has implementation for modern implementation of fake timer, we are still explicitly using jest-environment-jsdom-sixteen as the testing environment.

em/package.json

Line 120 in 5baf45d

"test": "react-scripts test --env=jsdom-sixteen",

It still does not pass modern implementation of fake timer to its environment. jest 26 ships with jsdom 16 by default. So we don't need to pass this environment here. I tested the lodash's debounce with upgraded react-scripts and jest and it's working with useFakeTimers('modern').

However tests are failing for pullQueue and dexie. I am working on it.

@shresthabijay
Copy link
Collaborator

shresthabijay commented Nov 27, 2020

@raineorshine dexie tests are failing after upgrade. Somehow Table.get is timing out when using fake timer. I isolated and tested get functions of db ex db.helpers.get in both versions of react-scripts. In the latest version it is timing out while using jest's fake timer (for both legacy and modern). I looked into Dexie codebase for any time dependent logic but I couldn't find any leads. But it's working without fake timers on both versions. So it cannot be problem with the mock indexed db.

@shresthabijay
Copy link
Collaborator

shresthabijay commented Nov 30, 2020

@raineorshine I found the reason for dexie not working with modern implementation of fake timer. With new jest , modern fake timer freezes setImmediate calls. fake-indexeddb uses setImmediate everywhere to execute call backs and handlers in next iteration of event loop. But dexie resolves its promises using these handlers.

          get: function (_a) {
                var trans = _a.trans, key = _a.key;
                return new Promise(function (resolve, reject) {
                    resolve = wrap(resolve);
                    var store = trans.objectStore(tableName);
                    var req = store.get(key);
                    req.onsuccess = function (event) { return resolve(event.target.result); };
                    req.onerror = eventRejectHandler(reject);
                });
            },

This is a code snippet from dexie library. Here get function returns a Promise which is resolved only when indexedDB ( in test case fake-indexeddb ) request's onsuccess handler executes. These handler calls are wrapped by setImmediate. Since jest modern fake timer seems to freeze setImmediate all the calls to the data-provider api timeout during test.

If we use legacy timer then lodash's debounce goes into infinite loop and if we use modern timer then fake-indexeddb calls times out.

I was tempted to use different timer implementation when needed throughout the test but it seems impossible to do so.

However I was was able to fix current dexie tests that were failing. I will be working on pullQueue test.

@raineorshine
Copy link
Contributor Author

I found the reason for dexie not working with modern implementation of fake timer. With new jest , modern fake timer freezes setImmediate calls. fake-indexeddb uses setImmediate everywhere to execute call backs and handlers in next iteration of event loop. But dexie resolves its promises using these handlers.

Nice work!

Let's make sure to document relevant links so we have them for our records. jestjs/jest#10221 seems to be the most relevant issue. If faking process.nextTick is broken and setImmediate uses it, then that could be the cause. Did you try modifying the dependency as suggested here to see if that's the problem? jestjs/jest#10221 (comment)

Is it only setImmediate that hangs, or also setTimeout(f, 0)? Since fake-indexeddb uses the setImmediate package, can it be mocked?

I also saw a few solutions that were suggested. Did you try these?

Let me know what you've tried and what you haven't, or if you get stuck and would like me to jump in. Thanks!

@shresthabijay
Copy link
Collaborator

@raineorshine I tried @sinonjs/fake-timers in this way.

import FakeTimers from '@sinonjs/fake-timers'

it('test', async () => {

  const clock = FakeTimers.install()
  initialize()
  await clock.runAllAsync()

  expect(1).toBe(1)
})

Everything get's initialized properly. All the timers gets flushed and promise callbacks are executed too. Including debounce calls. flushQueueDebounced gets executed too.

dexie tests are now working without needing to mock debounce. I will send a PR after refactoring. I am yet to work on pullQueue.

@shresthabijay
Copy link
Collaborator

@sinonjs/fake-timers is the best option to overcome shortcomings of jest fake timer.

Problems with jest fake timer

  • lodash's debounce doesn't work with Jest's legacy fake timer. It times out. But it works with new modern implementation of fake timer. Since debounce is used extensively in the codebase our option is to choose modern fake timer instead of mocking debounce implementation.

  • We need a way to flush promise callback in our test. We have a way to do so in 'legacy' fake timer.

    const flushPromise = () => new Promise(setImmediate)
    
    // This test passes
    it('test', async () => {
    
      jest.useFakeTimers('legacy')
    
      let a;
    
      Promise.resolve().then(() => {
        a = 1
      })
    
      // exhausts the promise callbacks
      await flushPromise()
      expect(a).toBe(1)
    })

    However with 'modern' implementation it doesn't work .

    /* However this test fails becuase of the modern fake timer implemenation */
    /* setImmediate is not called and promise is not resolved. You ask why ? Hmm good question ... */
    
    const flushPromise = () => new Promise(setImmediate)
    
    it('test', async () => {
    
      jest.useFakeTimers('modern')
    
      let a;
    
      Promise.resolve().then(() => {
        a = 1
      })
    
      // times out
      await flushPromise()
      expect(a).toBe(1)
    })

    This causes lot of trouble in testing for us. For example we need to call initialize before many tests. This requires executing all the debounced calls, promise callbacks to properly sync up store and db. Also all db calls gets timed out.

Why use Sinon Fake Timers

Sinon js fake timer mocks Date, works with lodash's debounce and flushes promise callbacks too. This solves the problems we are having with dexie and pullQueue tests. We don't have to use delay for pullQueue test either.

import FakeTimers from '@sinonjs/fake-timers'

it('With sinon js fake timer', async () => {
  const clock = FakeTimers.install()

  new Promise(resolve => setImmediate(() => {
    console.log('Inside setImmediate')
    resolve(true)
  })).then(() => {
    console.log('Flush Promise')
  })

  await clock.runAllAsync()
  expect(1).toBe(1)

  console.log('Test complete')
})

// 'Inside setImmediate'
// 'Flush Promise'
// 'Test complete'

With Jest 26 we are shipping a new implementation of fake timers based on @sinonjs/fake-timers.

https://jestjs.io/blog/2020/05/05/jest-26#new-fake-timers

Jest 26 timers are based on sinon fake timers but it doesn't seem to flush promises.

Usage in test

Sample of a dexie test with sinon js timers.

  it('persist newThought', async () => {

    const clock = FakeTimers.install()

    store.dispatch({ type: 'newThought', value: 'a' })

    // wait till all the pending timers are run and promise callbacks are executed. https://github.com/sinonjs/fake-timers#clockrunall--await-clockrunallasync
    await clock.runAllAsync()

    // Note: make sure to use real timer before awaiting db calls. Else it will time out. More on that in the caveat section below
    clock.uninstall()

    const parentEntryRoot = await getContext(db, [ROOT_TOKEN])

    expect(parentEntryRoot).toMatchObject({
      children: [{ value: 'a', rank: 0 }]
    })
  })

Caveat

Do not use fake timer before awaited db calls.

          get: function (_a) {
                var trans = _a.trans, key = _a.key;
                return new Promise(function (resolve, reject) {
                    resolve = wrap(resolve);
                    var store = trans.objectStore(tableName);
                    var req = store.get(key);
                    req.onsuccess = function (event) { return resolve(event.target.result); };
                    req.onerror = eventRejectHandler(reject);
                });
            },

This is a code snippet from dexie library. Here get function returns a Promise which is resolved only when indexedDB ( in test case fake-indexeddb ) request's onsuccess handler executes. These handler calls are wrapped by setImmediate.

With both modern jest timer and sinon fake timers this code times out

// jest_legacy.test.js
it('with jest legacy timer', async () => {
    // this test passes
    jest.useFakeTimers('legacy')
    await new Promise(setImmediate)
    expect(1).toBe(1)
})

// jest_modern.test.js
it('with jest modern timer', async () => {
    jest.useFakeTimers('modern')
    // this times out and fails
    await new Promise(setImmediate)
    expect(1).toBe(1)
})

// sinon.test.js
it('with sinon js', async () => {
    const clock = FakeTimers.install()
    // this times out and fails
    await new Promise(setImmediate)
    // similarly
    expect(1).toBe(1)
})

Similarly,

// This test times out and fails
it('persist newThought', async () => {

    const clock = FakeTimers.install()

    store.dispatch({ type: 'newThought', value: 'a' })

    // wait till all the pending timers are run and promise callbacks are executed. https://github.com/sinonjs/fake-timers#clockrunall--await-clockrunallasync
    await clock.runAllAsync()

    // clock.uninstall()

    // this will timeout just like the above examples
    const parentEntryRoot = await getContext(db, [ROOT_TOKEN])

    expect(parentEntryRoot).toMatchObject({
      children: [{ value: 'a', rank: 0 }]
    })
  })

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Testing and project configuration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants