fix($resource): Check if timeoutDeferred is null inside $cancelRequest

[chirag64]

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
Bug fix

What is the current behavior? (You can also link to an open issue here)
#16016

What is the new behavior (if this is a feature change)?

Does this PR introduce a breaking change?
No

Please check if the PR fulfills these requirements

• The commit message follows our guidelines: https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#commit-message-format
• Tests for the changes have been added (for bug fixes / features)
• Docs have been added / updated (for bug fixes / features)

Other information:

[gkalpak]

Can you please add some tests to make sure we don't accidentally break this in the future?

[namansancheti]

Hi @gkalpak,
I would like to create a new PR with the changes required as well as the tests added.
Could you please guide, how to write a test for this scenario ?

This is my first time contributing to this project, so kindly bear with me.
Thanks.

[chirag64]

That's where I got stuck too since I couldn't find any existing tests for cancelRequest. I thought I would spend some time digging into the code and figure it out but I couldn't 🙁

[gkalpak]

@chirag64, the $cancelRequest tests are (mostly) here. I would expect a new test along the following lines:

it('should not break when calling old `$cancelRequest` after the response arrives', function() {
  $httpBackend.whenGET('/CreditCard').respond({});
  
  var CreditCard = $resource('/CreditCard', {}, {
    get: {
      method: 'GET',
      cancellable: true
    }
  });
  
  var creditCard = CreditCard.get();
  var cancelRequest = creaditCard.$cancelRequest;
    
  $httpBackend.flush();
  
  expect(cancelRequest).not.toBe(noop);
  expect(cancelRequest).not.toThrow();
});

(Please, ensure that the test fails before your change in resource.js and succeeds with your change.)

@namansancheti, thx for stepping up. Let's leave this to @chirag64, since he is already working on it (and willing to take it further).

Don't get disappointed though 😃 We have lots of issues that are good candidates for first-time contributors here. (Just make sure you don't pick one someone's already working on.)

Always feel free to ask for further guidance/info/help. Welcome and have fun!

[namansancheti]

@gkalpak Thanks a lot for explaining how to write the test.
Sure, I shall leave this one to @chirag64 and work on some other issue.
Thanks.

[chirag64]

@gkalpak Thanks for the help with the test, I've pushed in a few changes. Let me know if I missed something out 🙂

[gkalpak]

What is this for?

[chirag64]

This was the piece of code mentioned in the Issue #16016 in the codepen to reproduce this issue. I suppose this is not needed since we'll be calling our function using the expect function?

[gkalpak]

If you call cancelRequest after flushing (as suggested in https://github.com/angular/angular.js/pull/16037/files#r127979323), then this shouldn't be needed (unless I am missing something).

[gkalpak]

This wouldn't fail before either. Please change to:

expect(cancelRequest).not.toBe(noop);
expect(cancelRequest).not.toThrow();

[chirag64]

So, I removed lines 2118 - 2121 and added not.toBe(noop) just above 2124, just like you mentioned in your comments. But now the tests are passing before and after the change I made in resource.js.

[gkalpak]

Have you also changed creditCard.$cancelRequest to cancelRequest?

[chirag64]

That's exactly what I missed, thanks 👍🏼

[gkalpak]

Thx! LGTM