Allow payload response to be a falsey but non-null or undefined value #126
Conversation
|
Could you add a unit test instead of a component/integration test? I don't think there's a reason we need to invoke all of the component/DOM logic to verify that this works correctly... Unless I'm missing something. I'd like to see a few test cases added, too: |
|
I totally agree. Good unit tests forthcoming. |
|
Fantastic 👍 |
| assert.equal(payloadWithFalseyNumber, 0); | ||
|
|
||
| const payloadWithNaN = service.handleResponse(200, {}, NaN); | ||
| assert.equal(isNaN(payloadWithNaN), true); |
There was a problem hiding this comment.
You can just do this as assert.ok. Not a merge-blocker, just a note.
|
I switched to using assert. what's with the appveyor build always failing? |
|
I'm not sure. It's not just your build, all of the PRs are failing like this. Not sure if it's an Appveyor issue or a Windows issue... I'm going to get set up in a Windows VM tonight and see what's going on, because I'm not comfortable pulling this if there is actually an issue on Windows. Hold tight and we'll get this pulled in as soon as I know what's up there. |
|
Actually, I just noticed that my PR from last night was fine on Appveyor.. Can you try rebasing off of the current master and pushing everything again? Maybe it was a problem with a dependency that is now updated. |
|
It seems like the tests are passing again, on the current |
|
I added you as a collaborator. Sorry was away for some time on a long vacation. |
|
No worries 😄 I'll try to get this finished up today. |
|
@SteelBurgher Can you check out my version of your branch over here? It should be exactly the same, but with the (same) tests written in Mocha and rebased off the current |
|
@alexlafroscia looks good, you can push. |
alexlafroscia
force-pushed
the
payload-checking-modification
branch
from
cb48094 to
3011250
Compare
|
Test are failing but it's due to this Ember bug so I'm not worried about it. |
No description provided.