toranb / ember-data-django-rest-adapter

An ember-data adapter for django web applications powered by the django-rest-framework
MIT License
154 stars 27 forks source link

See #57 - Fix serialization of multiword hasMany key on save operation #59

Closed g-cassie closed 10 years ago

g-cassie commented 10 years ago

This fixes https://github.com/toranb/ember-data-django-rest-adapter/issues/57

Please let me know if the test is okay. I thought a unit test would be sufficient for this purpose.

toranb commented 10 years ago

Nice work! Did you find the tests fairly straight forward? Any tech debt we should pay down in the future?

g-cassie commented 10 years ago

I struggled a bit with some strange behaviour with $.mockjax but eventually managed to get it working. I noticed that some of the tests are making httpRequests that get 404 responses. I'm not sure if this is really a big deal though.

The other thing I have found in the tests in my own project is that the App.reset() does not reset the store - so the tests end up layering on changes to the data, rather than resetting each time. I haven't fixed this in my own app, but here is an example of a test that resets the store on each run (it also uses an isolated container so it's a little funny): https://github.com/yapplabs/glazier/blob/master/test/app/controllers/add_pane_test.js

As an aside, I've found this repository to be a huge help when writing tests for Ember: https://github.com/ryanto/emberjs-nyc-hacker-hours-testing

toranb commented 10 years ago

@gordon--c I never thought to reset the store. I'll reach out to a few people this week to see how they reset the store (in addition to looking at the repo you listed).

Did you have an example that would throw an error anytime a 404 shows up? I really liked this idea btw -it helps keep your tests honest (less hidden http issues would be better long term for any project like this).

g-cassie commented 10 years ago

This implementation throws an error when you stub an endpoint that doesn't get queried: https://github.com/ryanto/emberjs-nyc-hacker-hours-testing/tree/master/08-mocking-xhr

I'm not sure how to throw an error on a 404 - I think if $.mockjax stored a list of all requests made (whether they were mocked or not) then you could assert against it in teardown - but it doesn't look like it does that. For what it's worth, I noticed the 404 errors by running karma in Chrome on continuos mode, opening the debug panel and then watching the console as the tests ran.

craigteegarden commented 10 years ago

I think the issue is we are performing async operations but we are not using the asyncTest (http://api.qunitjs.com/asyncTest/) which is causing tests to overlap, which leaks state between tests.

Also, I believe that all of the 404s for ajax calls occur whenever we don't explicitly stub a request with stubEndpointForHttpRequest.

toranb commented 10 years ago

Agreed -I spoke with a few of the core guys and they said that App.reset() throws out the container (thus -no shared state should occur across tests).

@craigteegarden should we just add the asyncTest to each callback then? How does this work when the find methods are invoke in the sample app (do we need them in the app.js each time we make an async request) ?

g-cassie commented 10 years ago

I looked closer at it and it seems you are right about the store. I had been told that it does not get cleared when using a RESTAdapter from App.reset() by one of the core members at a meetup but I must have misunderstood.

My understanding (though also probably mistaken) is that asyncTest is not necessary when your tests rely entirely on DOM manipulations because in that case the waiting is built in to the router.