optimizely / nuclear-js

Reactive Flux built with ImmutableJS data structures. Framework agnostic.
https://optimizely.github.io/nuclear-js/
MIT License
2.23k stars 141 forks source link

Race condition when testing #163

Closed jakecraige closed 9 years ago

jakecraige commented 9 years ago

I tried following your guide on testing and am seeing a race condition between the assertion happening and the store receiving an action type that updates it.

My test looks like:

actions.toggleShowMorePresets();
expect(reactor.evaluate(getters.showMorePresets)).to.eql(true);

Here's the debugging output where I've put some logs to show the order of things:

karma_debug_runner

Is there a way to resolve this? Somehow make nucler trigger things synchronous in test?

More info:

action:

toggleShowMorePresets: function() {
    reactor.dispatch(TOGGLE_SHOW_MORE_PRESETS);
}

store:

  initialize() {
    this.on(TOGGLE_SHOW_MORE_PRESETS, state => {
      return state.updateIn(['sections', 0, 'showMorePresets'], showMore => {
        return !showMore;
      });
    });
  }
jordangarcia commented 9 years ago

Interesting.

Nuclear should be doing everything synchronously.

Where does store registration happen.

Would you mind posting the full function block in your test code?

jakecraige commented 9 years ago

@jordangarcia Sure

Store registration in module index

import reactor from 'reactor';
import actions from './actions';
import getters from './getters';
import ResizePanelStore from './stores/ResizePanelStore';

reactor.registerStores({
  ResizePanel: ResizePanelStore
});

export default {
  actions: actions,
  getters: getters
};

Test looks like:

import reactor from 'reactor';
import {actions, getters} from 'modules/ResizePanel';

describe('ResizePanel', () => {
  describe('actions', () => {
    describe('#toggleShowMorePresets', () => {
      it('should toggle from false to true and back', () => {
        expect(reactor.evaluate(getters.showMorePresets)).to.eql(false);
        actions.toggleShowMorePresets();
        expect(reactor.evaluate(getters.showMorePresets)).to.eql(true);
      });
    });
  });
});

It's running in Chrome with Karma + Mocha + Chai

jakecraige commented 9 years ago

For more info, I've created a function using observers to verify it is a race condition and it does pass.

helper fn:

reactor.nextValue = function(keyPath) {
  return new Promise((resolve, _reject) => {
    var unwatch;
    unwatch = reactor.observe(keyPath, (...args) => {
      unwatch();
      resolve(...args);
    });
  });
};

Test:

      it('should toggle from false to true and back', () => {
        expect(reactor.evaluate(getters.showMorePresets)).to.eql(false);

        actions.toggleShowMorePresets();

        return reactor.nextValue(getters.showMorePresets).then(newValue => {
          expect(newValue).to.eql(true);

          actions.toggleShowMorePresets();
          return reactor.nextValue(getters.showMorePresets);
        }).then(newValue => {
          expect(newValue).to.eql(false);
        });
      });
jordangarcia commented 9 years ago

Oh man, that's pretty gnarly.

Your test code looks fine to me.

Mind posting what your store looks like.

There is an issue with the evaluator and occasional collisions that cause stale evaluate values. See #140.

I am in the process of refactoring the evaluator to fix this, not 100% sure that your issue is related to this but seems like the mostly likely case.

jakecraige commented 9 years ago

@jordangarcia Sure, like so:

import { Store, toImmutable } from 'nuclear-js';
import reactor from 'reactor';
import getters from '../getters';
import { TOGGLE_SHOW_MORE_PRESETS } from '../actionTypes';

function toggleShowMorePresets(state) {
  return state.updateIn(['sections', 0, 'showMorePresets'], showMore => {
    return !showMore;
  });
}

export default Store({
  getInitialState() {
    return toImmutable(require('json!yaml!../state.yml'));
  },

  initialize() {
    this.on(TOGGLE_SHOW_MORE_PRESETS, toggleShowMorePresets);
  }
});
jordangarcia commented 9 years ago

hmmmm.

Can you try something crazy.

can you make the toggleShowMorePresets function return a string like 'foobar' and see if the race condition still exists.

If it doesn't then this is indeed related to a stale cached value like in #140. If not then it may be something deeper.

jakecraige commented 9 years ago

When changing it to something like:

  return state.updateIn(['sections', 'index', 'showMorePresets'], showMore => {
    return showMore === "true" ? "false" : "true";
  });

I see the same behavior :/ Is that what you had in mind?

note: I've changed how I'm accessing the getter during this time which is why the keyPath is different in this one.

jakecraige commented 9 years ago

@jordangarcia Crap, I figured it out. We had a custom Reactor class that dispatched changes on requestAnimationFrame and that was causing them to be triggered async.

Thanks for the quick assistance though. Apologies for having you look into a non-problem.