TestUtils for Actions

[wavded]

I see there are TestUtils being worked on for Stores. Has any thought been given to Actions? I understand the rationale behind wrapping actions as a safeguard but the "magic" makes it harder to test.

[acdlite]

Yes, definitely! Haven't figured out the best API for this yet, though. Any suggestions?

My initial thought is something like

let result = TestUtils.captureAction(action);

which executes the action and returns the result, rather than dispatching it. Thoughts? I don't know if I like the name.

[acdlite]

This just occurred to me, but another option would be to only enable the "magic" action wrapping if/when the actions are added to a Flux instance. So if you create an Actions instance with new, performing an action will return a value without dispatching — instead of the current behavior, which is to throw an error.

[wavded]

I'm mixed if behavior changes on how its used (magic enabled in some context, not in others). I do like the idea of testing it by using the methods directly on the instance. That feels most natural and clear to me. What if it returned normally everywhere as you'd expect but if we detect its used in a "bad" manner we can throw up a warning (kinda like other methods do)?

[acdlite]

I would love to just throw a warning, but I don't know of any way to detect it...

To me, the "magic" is that the return value is automatically dispatched. I think that's useful, because it means you don't have to do this.dispatch() at the end of every action. The usefulness of the other part — not returning to the caller — is up for debate. I like it because it prevents Flux novices from using actions as if they were generic helper / util functions. But I'm willing to give this up if it makes testing simpler.

[wavded]

Ahh... I see. I do like not having to think about dispatching, as it seems like just plumbing and boilerplate code you shouldn't need to worry about. Maybe a TestUtils method is best then?

[tappleby]

I think the TestUtils option is the best bet, otherwise we need to delay the wrapping of actions until they get attached to the flux instance (bigger change + action ids wont be available right away).

Its probably safe to update Actions::_dispatch to return since the Flux::dispatch has no return.

With the above change we would just need a test utils function (not sure a good name) that sets the dispatch functions on an action class to return the body/promise:

actions.dispatch = (actionId, body) => body;
actions.dispatchAsync = (actionId, promise) => promise;

perhaps:

let actions = TestUtils.mockActions(ActionClass, ...constructorArgs);
let body = actions.action();

[tappleby]

Or maybe TestUtils.createActions to mirror the Flux api.

[acdlite]

@tappleby You wouldn't have to delay the wrapping. Wrap immediately and put an if statement before these lines: https://github.com/acdlite/flummox/blob/master/src/Actions.js#L54-L59

if (!this._hasFlux) return body;

[tappleby]

That could work, you would lose the You've attempted to perform the action ..., but it hasn't been added to a Flux instance. if we did that.

[acdlite]

In this scenario, we'd move that error from _dispatch() and _dispatchAsync() into the wrapped action itself, and make it a warning.

I think I'm leaning towards a TestUtils method, too, but I want to at least consider this. The actions magic, as @wavded accurately put it, is the part of Flummox I'm least sure of. This has the potential to demystify it a bit without losing the auto-dispatching functionality.

[tappleby]

I do like the idea of just a warning w/ the body being returned.

I have mixed feelings about the auto dispatch functionality/magic, its not immediately clear what happens when you return a value just by looking at the code.

If there was a way to detect the current executing action something like this would be explicit:

action(arg1, arg2) {
  this.dispatch({ arg1, arg2 });
}

[acdlite]

That would be possible, but I really don't want to have to do this.dispatch() at the end of every action. I agree the explicitness is clearer, but it also feels... redundant.

[wavded]

It seems like you've guys went back and forth on this so if I missed something, I apologize, but what if the Action classes themselves did not add auto dispatching when instantiated but rather that was added later when you did flux.createActions(...) ? Or is that too delayed? I guess I'm unfamiliar with workflows that would cause problems.

[acdlite]

@wavded That's definitely an option. The more I think about this, though, I think the best option is to just always return the dispatched value to the caller, in addition to auto-dispatching. This gives us easy testability, and the convenience of auto-dispatching, and it's probably less confusing for new Flummox users.

[wavded]

Works for me :)

[acdlite]

Released as part of v2.12.0 :)

[wavded]

Thanks for taking a look. Although, I don't think we are quite there yet. Now if I create a new Actions instance and try to call an action like actions.startOver(), I get:

ReferenceError: You've attempted to perform the action AppActions#startOver, but it hasn't been added to a Flux instance.

I was hoping not to have to set up a Flux instance to test Actions in isolation. Thoughts?

[acdlite]

Blah, forgot to change that to a warning. I'll fix here in a bit.

On Fri, Feb 27, 2015 at 5:47 PM, Marc Harter [email protected]
wrote:

Thanks for taking a look. Although, I don't think we are quite there yet. Now if I create a new Actions instance and try to call an action like actions.startOver(), I get:

ReferenceError: You've attempted to perform the action AppActions#startOver, but it hasn't been added to a Flux instance.

I was hoping not to have to set up a Flux instance to test Actions in isolation. Thoughts?

Reply to this email directly or view it on GitHub:
#46 (comment)

[acdlite]

@wavded Fixed in 2.12.1

[wavded]

@acdlite thanks much!

[wavded]

@acdlite hate to bug you again on this one but it throws still if its asynchronous:

You've attempted to perform the asynchronous action AppActions#performGeocode, but it hasn't been added to a Flux instance

[acdlite]

Haha I'm having a rough day I guess

On Fri, Feb 27, 2015 at 6:41 PM, Marc Harter [email protected]
wrote:

@acdlite hate to bug you again on this one but it throws still if its asynchronous:

You've attempted to perform the asynchronous action AppActions#performGeocode, but it hasn't been added to a Flux instance

---

Reply to this email directly or view it on GitHub:
#46 (comment)

[wavded]

:) no worries, it happens.

[acdlite]

Fixed in 2.12.2... I think :D

[girvo]

Hey @acdlite -- as of 4.0.0alpha this warning for async actions still totally exists!

[12:09:18] Using gulpfile ~/Work/Projects/viewfinder-1.3/code/gulpfile.js
[12:09:18] Starting 'test'...


  actions/APIActions
You've attempted to perform the asynchronous action APIActions#sendRequest, but it hasn't been added to a Flux instance.

The test case:

   ...

    before(() => {
        APIActions = rewire('../../../src/shared/actions/APIActions');

        EclipseMock = sinon.stub();
        EclipseMock.sendRequest = sinon.stub().resolves({
            hello: 'world'
        });

        revert = APIActions.__set__('api', EclipseMock)
    });

    it('should await on the response from the API', () => {
        const actions = new APIActions();
        let result = actions.sendRequest();

        EclipseMock.sendRequest.should.be.called.equal(true);
        return result.should.be.fulfilled.then((res) => {
            console.log(res)
        });
    });

And the action itself:

let Actions = require('flummox').Actions;
let EclipseClient = require('../utils/EclipseClient');
let api = new EclipseClient();

export default class APIActions extends Actions
{
    async sendRequest(reqParams) {
        const response = await api.sendRequest(reqParams);
        return await response;
    }
}

Any ideas on how to hide the warning?

[lettertwo]

I see the warning when testing sync actions, too, FWIW.

[themouette]

I'm wondering:

Wouldn't it make sense to have actions wrapped in the flux instance createActions method.

IMHO this "convert Promise to dispatcher" thing is the Flux responsibility, not the actions'.
From my understanding, Actions should not have any knowledge of the Flux instance though unit testing an action should not require it neither.

WDYT?

[acdlite]

The warning has been removed from master.

@themouette Yes, I agree. We're actually going to remove the Actions class in the next release and require users to use plain JavaScript objects. As part of that conversion, we'll move the wrapping stuff to the Flux class.

[themouette]

that sounds like great news! Thank you.

BTW I created a simple actions test helper. This is a very naive implementation, but it might help someone.

Of course, feel free to enhance :)

Sample usage:

// create a test action
let action = createActions(SomeActions, extra, args);
// register handlers as actions methods cannot be called directly
let method = action.register(action.myAwesomeAction, successHandler, errorHandler);
// call the decorated method
method(some, args);

// For async methods
let method = action.registerAsync(
  action.myAwesomeAction,
  beginHandle,
  successHandler,
  errorHandler,
  always
);
// call the decorated method
method(some, args);