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

[WIP] Add andFail() method to handleFind() #85

Merged
merged 1 commit into from
Jun 2, 2015
Merged

[WIP] Add andFail() method to handleFind() #85

merged 1 commit into from
Jun 2, 2015

Conversation

alexanderjeurissen
Copy link
Contributor

This pullrequest implements issue #84 it adds the same andFail() method chaining functionality that is already implemented for PUT and DELETE requests to the handleFind() test-helper.

TODO:

  • fix specs
  • update comments to reflect new method

@alexanderjeurissen
Copy link
Contributor Author

@danielspaniel somehow the specs are failing, I'm not sure why but the error callback of the promise isn't resolved.

not ok 105 PhantomJS 1.9 - FactoryGuyTestHelper with DS.RESTAdapter : #handleFind failure with andFail method
    ---
        actual: >
            false
        expected: >
            true
        message: >
            failed, expected argument to be truthy, was: false
        Log: >
            INFO: requestConfig set
            INFO: andFail is called
            RESPONSE: {"created_at":null,"description":"Text goes here","camelCaseDescription":null,"snake_case_description":null,"company":null,"group":null}
    ...

As you can see the response is as what we'd expect in case the andFail() was not present. so somehow the succeed flag isn't set properly, or the handler is called before the andFail() method has set succeed to false.

I'm having the feeling that I miss something very obvious, could you weigh in on this ?

};

this.andFail = function (options) {
console.log('INFO: andFail is called');
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ugh forgot this one.. will remove it.

@alexanderjeurissen
Copy link
Contributor Author

@danielspaniel that won't do, because somehow the promise resolves the success callback, without the done() statement in the success callback the test will hang, resulting in the following error:

not ok 105 PhantomJS 1.9 - FactoryGuyTestHelper with DS.RESTAdapter : #handleFind failure with andFail method
    ---
        actual: >
            null
        message: >
            Test timed out
        Log: >
    ...

however only adding the done() call (to let Qunit know the asynchronous part of the spec is finished) will result in the following error message:

not ok 105 PhantomJS 1.9 - FactoryGuyTestHelper with DS.RESTAdapter : #handleFind failure with andFail method
    ---
        actual: >
            null
        message: >
            Expected at least one assertion, but none were run - call expect(0) to accept zero assertions.
        Log: >
    ...

which is to be expected, the only weird thing is that the promise won't call the error callback. Even if i comment out all code in the handler and replace it with something like this:

this.handler = function () {
    this.responseText = {};
    this.status = 500;
}

still i get a valid record back in the response parameter of the success callback, so it seems the handler is never called

@danielspaniel
Copy link
Collaborator

right .. well, the problem then seems to be that the request should fail, and it's not, which means maybe your request mock is not catching the request? can not exactly tell without running this test myself and checking it out. If you can't figure it out today, I will work on it tomorrow.

@alexanderjeurissen
Copy link
Contributor Author

yeah seems that for whatever reason the request is not catched by $.mockAjax I'll look into it some more, when I'm stuck I'll let you know

@danielspaniel
Copy link
Collaborator

Hi @alexanderjeurissen .. I am working on this now, and I see the problem.
You could help me a ton by telling be your use cases for handleFind ( success and failure ) , since I may change this functionality around based on how it is really used.
I can see it for testing reload .. of model you have. But for the rest of the cases I am a bit unclear.

@alexanderjeurissen
Copy link
Contributor Author

Hi @danielspaniel , I've been busy lately and haven't found time to fix this PR. I'm curious as to what the problem was ?

I'll gladly provide you with some use-cases / specs where we are currently using the handleFind method.

usecase 1 (handleFind success):

test('create new path and redirect', function () {
  expect(1);

  visit('/paths/new');
  fillIn('.path-header > .inner > input', 'Javascript Ninjas');
  fillIn('.path-header > .inner > textarea', 'Become a ninja at JavaScript');

  click('button.button-save');

  testHelper.handleCreate('path');
  testHelper.handleFind('path', {
    id: 1
  });

  andThen(function () {
    expectRouteToBe('path.index');
  });
});

this spec uses the handleFind to make sure the store contains a path because after saving a new path the app redirects to the index route to show the newly created path.

usecase 2 (handleFind failure):

test('renders not-found page for nonexisting ids', function () {
  expect(1);

  visit('/products/1');

  testHelper.handleFind('product', {id: "1"});

  andThen(function () {
    expectRouteToBe('not_found'); //custom helper that checks if currentPath() equals X
  });
});

here we want to test if our router correctly handles redirecting to a not_found aka 404 route in case a resource is requested that does not exist.

@danielspaniel
Copy link
Collaborator

Thanks @alexanderjeurissen .. these examples were very helpful. What I see is that I have made some mistakes in how I assumed handleFind would be used. I was creating a model and placing it in the store in the handleFind method ( which is why the ajax call is never made.)
So, for your first example that would actually be bad, since you don't want another model, you want the one you just created.
But in that case actually .. you don't need to say handleFind after a handleCreate, since the store will ( after the handleCreate/ or createRecord/ save events will now have that model you just created.
No need to handleFind, since the store already knows it has it and will NOT do an ajax call to get it.
Therefore you don't need that handleFind ( correct me if I am wrong .. or you find that you need it )

The second case is what I figured would be the traditional one, and knowing that .. I will now fix the and fail to work in these traditional cases.

I was trying to make handleFind too fancy before, and I will strip it down to be more basic.

@danielspaniel danielspaniel merged commit 7c90a2d into adopted-ember-addons:master Jun 2, 2015
@danielspaniel
Copy link
Collaborator

@alexanderjeurissen, version 1.0.10 has the andFail method working with handleFind. Thanks for getting the ball rolling on this one. You helped me to clean this method up from the mess it was.

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

Successfully merging this pull request may close these issues.

2 participants