Improve test setup #92

aars · 2016-08-25T14:08:55Z

This PR is intended to review/discuss changes.

…ype from toString()

aars · 2016-08-25T14:10:40Z

addon/models/resource.js

@@ -116,7 +116,7 @@ const Resource = Ember.Object.extend(ResourceOperationsMixin, {
    @method toString
  */
  toString() {
-    let type = this.get('type') || 'null';
+    let type = singularize(this.get('type')) || 'null';


This makes toStrings more predictable and easier to test.

aars · 2016-08-25T14:20:20Z

Cleans and improves some/most tests. Replaces a lot of handwritten strings (relationUrl etc) to test against with references to the actual urls from the used fixtures/mockdata.

Adds actual calling of promise.then() in a lot of tests. I'm unclear on why this was already done on some tests but not on others. Anyway, if there ever is a mock-server to test against this will actually become async and important.

Uses models defined in dummy app (I'm not sure if I broke that dummy app with the changes in models. Haven't looked at that yet).

pixelhandler · 2016-08-27T17:06:32Z

tests/unit/adapters/application-test.js

  const adapter = this.subject({type: 'posts', url: '/posts'});
  sandbox.stub(adapter, 'fetch', function () { return RSVP.Promise.resolve(null); });
  let resource = this.container.lookup('model:post').create(postMock.data);
  sandbox.stub(adapter, 'cacheRemove', function () {});
-  Ember.run(function() {
+  Ember.run(() => { // Why is this called inside a Ember RunLoop? Does that imply async?


I forget when I needed the run loop, but typically I'm expecting some backburner queues to be processed.

I'm pretty sure it was to be sure https://github.com/pixelhandler/ember-jsonapi-resources/blob/master/addon/adapters/application.js#L188 resource.destroy(); is completed.

Since theoretically stuff could break within the runloop I'll use another assert.expect() and async() here so we know when it does.

pixelhandler · 2016-08-27T17:18:49Z

@aars I like the changes. I replied to a few questions in the code comments. All the tests pass, (using ember try as well).

Perhaps we merge in after addressing the questions in the code comments.

aars · 2016-08-29T10:47:27Z

@pixelhandler Replies and updates. Will continue with other tests now.

Please have a look at my changes to find(), the PR diff makes it a bit confusing since it's actually a complete rewrite of the function. https://github.com/aars/ember-jsonapi-resources/blob/improve-test-setup/addon/adapters/application.js#L48-L69

I ended up not even testing for number. Just casting to string whenever an apparent id is found. I think this covers everything (including undefined and 0 and "0")

aars · 2016-08-29T12:45:43Z

@pixelhandler Regarding casting ids to strings. If I could direct your attention at this code, written by you: https://github.com/pixelhandler/ember-jsonapi-resources/blob/master/tests/unit/mixins/resource-operations-test.js

It uses numbers as ids everywhere. You use it consistently in this file, so no test is broken... but... this is incorrect usage of the lib/spec, in your lib, by you. It will break as soon as you, per spec, try to look up a relation by id using a string.

Can I use this to strengthen my argument about easy pitfalls and annoying users? 😝

PS: The code in this test creates invalid JSONAPI payloads to the server as well, since ids are never transformed.

pixelhandler · 2016-08-29T16:43:19Z

@aars when we cast to string for fetching, we should also cast to string when we deserialize too.

pixelhandler · 2016-08-30T06:33:07Z

addon/adapters/application.js

-      if (options.id) {
-        return this.findOne(options.id, options.query);
+    // Collect id and query from options (if given).
+    // ids are cast to string to allow number input.


perhaps instead of "to allow" we could say to "ensure id is a Strings (which spec requires)"

Many better.

aars · 2016-08-30T06:39:43Z

@pixelhandler Casting to string when deserializing is on the other side of "the spectrum" though, as in; the current goal is to improve JSONAPI compliancy for "users" of this library, programmers (people) who feed it data, where mistakes like numerical ids can easily happen. On the other side is the server, which should never have sent a numerical id to begin with. If we're casting to string when deserializing we're only adding in support for broken JSONAPI endpoints... No? What do you think?

Not that I'm really against it. I would however add a warning message about the server sending invalid payloads (edit: I am now against it! 😄 see some line comment)

pixelhandler · 2016-08-30T06:40:25Z

tests/unit/adapters/application-test.js

@@ -189,6 +221,10 @@ test('#findRelated', function(assert) {
  });
 });

+// TODO: This test is broken in an odd fashion. Ends up somehow calling `addRelationship`


This was addressed in PR #94

pixelhandler · 2016-08-30T06:49:32Z

@aars casting in create is good for developers and perhaps a warning if server sends ids and numbers could help. The think is the expectation of this library is that that server follows the JSONAPI 1.0 spec. It may not be necessary to assert that the IDs in a JSON payload are strings.

Here is where the deserialization begins, https://github.com/pixelhandler/ember-jsonapi-resources/blob/master/addon/mixins/fetch.js#L148-L150 And in the serializer we could add an assert for the id type in here https://github.com/pixelhandler/ember-jsonapi-resources/blob/master/addon/serializers/application.js#L208-L233 but seems fine to just trust that the server is doing it's part.

aars · 2016-08-30T06:53:44Z

I'm on my second sip of coffee, I wasn't fully awake when I wrote about deserialize supporting broken servers. Still am not, but as you noticed and commented, the casting to string in create is enough to also handle casting when/after deserializing. If we're not adding code specific to deserializing, it's no longer fitting to give a warning about numerical-ids from the server. I'd say we're done with that question. (edit: as in, we trust the server. Or, that's not our problem. We can still handle it)

…ype from toString()

aars · 2016-08-30T08:51:04Z

I have rebased/merged. I will squash the commits later today.

edit: I'm having a horrible time rebasing/squashing this. Can we consider this PR and commit history for merging?

If you prefer these commits squash I'll cherry-pick them on a new branch and create a new PR.

…ts. Improves comment

aars · 2016-09-01T09:47:43Z

When all is 🎉 I have this PR ready to fix/improve working with relationship resource types: realxsnl#1 (goes on top of this PR).

pixelhandler · 2016-09-01T18:20:20Z

@aars there is a "Squash and merge" option so I can just do that, no need for you to squash.

aars added 9 commits August 23, 2016 15:18

deepEqual actual data

51f8fed

Use resource from dummy app model definition.

7382a95

Add unsafe-inline and unsafe-eval CSP

0fbba50

singularize type in toString()

27e37ab

Use dummy app models. Person->Employee->Supervisor.

2c0da1d

Add person and supervisor models. Use plural types.

f127f95

Supervisor extends Employee (use EmployeeService). Expect singulare t…

6eed532

…ype from toString()

Test data-structure, not json string.

4e00acc

clean up of adapter/application unit tests

c79e922

aars reviewed Aug 25, 2016
View reviewed changes

pixelhandler reviewed Aug 27, 2016
View reviewed changes

aars added 2 commits August 29, 2016 12:25

#find() -> casts ids to string. Changes tests accordingly.

85cb605

Improve adapter error response handling test

80a1804

removes console.log debug statement

45073d2

Cast ids to strings at 'public' entrypoints

fc3957f

pixelhandler reviewed Aug 30, 2016
View reviewed changes

aars added 17 commits August 30, 2016 10:01

deepEqual actual data

3aa664f

Use resource from dummy app model definition.

1a0eed9

Add unsafe-inline and unsafe-eval CSP

8e6e151

singularize type in toString()

2fb3127

Use dummy app models. Person->Employee->Supervisor.

3006294

Add person and supervisor models. Use plural types.

0a8a38f

Supervisor extends Employee (use EmployeeService). Expect singulare t…

c663d0a

…ype from toString()

Test data-structure, not json string.

7992141

clean up of adapter/application unit tests

c5df288

#find() -> casts ids to string. Changes tests accordingly.

165db1d

Improve adapter error response handling test

cfb0a5f

removes console.log debug statement

70b31e0

Cast ids to strings at 'public' entrypoints

246afcb

pre-merge

af0754c

merged/updated fix for incomplete findRelated test

190d364

merged

c28e08e

improves comment

f1b8095

cast id to string on adapter create- and deleteRelationship. Adds tes…

b54fc8c

…ts. Improves comment

pixelhandler merged commit 62386a5 into pixelhandler:master Sep 1, 2016

aars deleted the improve-test-setup branch September 23, 2016 10:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve test setup #92

Improve test setup #92

aars commented Aug 25, 2016

aars Aug 25, 2016

aars commented Aug 25, 2016

pixelhandler Aug 27, 2016

aars Aug 29, 2016

pixelhandler commented Aug 27, 2016

aars commented Aug 29, 2016 •

edited

Loading

aars commented Aug 29, 2016 •

edited

Loading

pixelhandler commented Aug 29, 2016

pixelhandler Aug 30, 2016

aars Aug 30, 2016

aars commented Aug 30, 2016 •

edited

Loading

pixelhandler Aug 30, 2016

pixelhandler commented Aug 30, 2016

aars commented Aug 30, 2016 •

edited

Loading

aars commented Aug 30, 2016 •

edited

Loading

aars commented Sep 1, 2016

pixelhandler commented Sep 1, 2016

Improve test setup #92

Improve test setup #92

Conversation

aars commented Aug 25, 2016

aars Aug 25, 2016

Choose a reason for hiding this comment

aars commented Aug 25, 2016

pixelhandler Aug 27, 2016

Choose a reason for hiding this comment

aars Aug 29, 2016

Choose a reason for hiding this comment

pixelhandler commented Aug 27, 2016

aars commented Aug 29, 2016 • edited Loading

aars commented Aug 29, 2016 • edited Loading

pixelhandler commented Aug 29, 2016

pixelhandler Aug 30, 2016

Choose a reason for hiding this comment

aars Aug 30, 2016

Choose a reason for hiding this comment

aars commented Aug 30, 2016 • edited Loading

pixelhandler Aug 30, 2016

Choose a reason for hiding this comment

pixelhandler commented Aug 30, 2016

aars commented Aug 30, 2016 • edited Loading

aars commented Aug 30, 2016 • edited Loading

aars commented Sep 1, 2016

pixelhandler commented Sep 1, 2016

aars commented Aug 29, 2016 •

edited

Loading

aars commented Aug 29, 2016 •

edited

Loading

aars commented Aug 30, 2016 •

edited

Loading

aars commented Aug 30, 2016 •

edited

Loading

aars commented Aug 30, 2016 •

edited

Loading