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

added ember-testing integration test for issue #2997 #3026

Merged
merged 1 commit into from
Aug 25, 2013
Merged

added ember-testing integration test for issue #2997 #3026

merged 1 commit into from
Aug 25, 2013

Conversation

toranb
Copy link
Contributor

@toranb toranb commented Jul 21, 2013

Erik

I added the failing scenario as a test so you have something to start with. Note- it's not 100% functional because I didn't know how you would want to pull in jQuery for the actual test (or if you would prefer to avoid it at all costs and do something else instead).

Either way, you can take parts from what I have here or roll your own. I just wanted to have an example up to help move this along so we can hit 1.0 :)

Thanks again for all the help on this issue

Toran

@stefanpenner
Copy link
Member

for those of us unfamiliar with the issue can u describe the failure in words as well?

@toranb
Copy link
Contributor Author

toranb commented Jul 23, 2013

When I was integration testing with ember.js RC5 and I wanted to hit the "/" route of my app using visit, the route handler would invoke the model function both when the app was started and when I hit the "visit" line in my test code. This allowed me to modify what would occur in the model hook just before calling "visit"

In the test I attached, I stub out what the xhr should return before calling visit("/"). In RC5 this worked great but for some reason in RC6 it won't fire the model hook a second time (as it already fired when the app was loaded I assume).

The happy-path should be that when I want to visit a route, the model method will fire when my test code is executed (so I can setup the state of the application for example).

Thank you in advance

@toranb
Copy link
Contributor Author

toranb commented Aug 15, 2013

For anyone watching this issue, it's still broken in ember.js RC7

@toranb
Copy link
Contributor Author

toranb commented Aug 18, 2013

I updated the unit test, it helped me find the offending commit that broke this behavior

5730c31

@mattjmorrison
Copy link

I just ran into this issue also. Thanks @toranb for getting this issue out there!

@taddeimania
Copy link

+1

@williamsbdev
Copy link

I'd like to see this fixed so we can upgrade from RC5. +1

@imtapps
Copy link

imtapps commented Aug 23, 2013

👍

@JarrodCTaylor
Copy link

Would also be very interested in a fix!

@toranb
Copy link
Contributor Author

toranb commented Aug 25, 2013

@stefanpenner I just updated the test after our pairing session and it's GREEN my friend. Can you pull it in before you go live w/ RC8 ?

@stefanpenner
Copy link
Member

absolutely.

I think the implicit 'visit("/")' that occurs during testing today isn't ideal. Rather requiring an explicit visit might be better. Although that comes with it's own caveats, it may be the easier to teach and explain.

stefanpenner added a commit that referenced this pull request Aug 25, 2013
added ember-testing integration test for issue #2997
@stefanpenner stefanpenner merged commit 122fc6e into emberjs:master Aug 25, 2013
@mattjmorrison
Copy link

Thanks @toranb and @stefanpenner for this. I do have questions / comments.

Is the testing API for getting my Ember app to an initial state that in my setup I call:

App.setupForTesting();
App.reset();
App.deferReadiness();
App.injectTestHelper();

then inside each test I call:

App.advanceReadiness();

and in my teardown I call:

App.revmoveTestHelpers();
Ember.$('#ember-testing').remove();
Ember.run(App, App.destroy);
App = null;

I think this test is a good demonstration of how to reset your Ember app, however I do not think it is a very real world-like example. The App = null; and the fact that the ember app is created inside of the test's setup is something that will never happen for a real Ember application.

I'm not sure that I can write the test that I want to be able to write with the implicit visit('/'). Here is an example of the test that I want to be able to write (using the mockjax library.)

test("shows list of user groups", function(){
    jQuery.mockjax({
        type: 'GET',
        url: '/groups',
        status: 200,
        dataType: "json",
        responseText: {'groups': [
            { id: 1, name: 'Group One'}
        ]}
    });

    visit('/').then(function(){
        var group  = find(".groups li:eq(0)").text();
        equal(group, "Group One", group + " was incorrect");
    });
});

Currently (as of rc7), I have not been able to write this test because (I believe) of the implicit visit('/') even after playing around with some of the methods used in this pull request's test example.

I do not want to override the find method in my test, because I want to actually test that it is working correctly, I want to just fake the data that would be coming back to the server.

Is there something that I'm missing that will be able to reset the entire state of my Ember app so that when I visit('/') in a test it will actually run the whole stack?

@stefanpenner
Copy link
Member

@mattjmorrison i think the implicit visit that occurs today on App.reset is somewhat of an anti-pattern, and will likely soon vanish.

  • You should not need to override find. Mocking the async at any level should be sufficient.

What likely needs to happen today.

setup: function(){
  Ember.run(function(){
    App.reset();
    App.deferReadiness(); // prevents the app for booting, and starting to route.
  });
};

test("shows list of user groups", function(){
    jQuery.mockjax({
        type: 'GET',
        url: '/groups',
        status: 200,
        dataType: "json",
        responseText: {'groups': [
            { id: 1, name: 'Group One'}
        ]}
    });

   // app has not finished booting yet.
   Ember.run(App, 'advanceReadiness');
   // Assuming all async was correctly stubbed out, your app should now be in '/

   // please note, in your example u where visiting / a second time.
   var group  = find(".groups li:eq(0)").text();
   equal(group, "Group One", group + " was incorrect");
});

Proposal for the near future: (explicit visit)

setup: function(){
  App.reset();
};

test("shows list of user groups", function(){
   // no implicit 'visit' has happened
   // so feel free to mock
  jQuery.mockjax({
        type: 'GET',
        url: '/groups',
        status: 200,
        dataType: "json",
        responseText: {'groups': [
            { id: 1, name: 'Group One'}
        ]}
    });

    // explicit visit required
    visit('/').then(function(){
      var group  = find(".groups li:eq(0)").text();
       equal(group, "Group One", group + " was incorrect");
    });
});

@toranb
Copy link
Contributor Author

toranb commented Aug 27, 2013

I've added a test to show the before and after of this scenario to show that no state is being leaked across tests

https://github.com/emberjs/ember.js/pull/3212/files

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.

None yet

8 participants