ToothlessGear / node-gcm

A NodeJS wrapper library port to send data to Android devices via Google Cloud Messaging
https://github.com/ToothlessGear/node-gcm
Other
1.3k stars 208 forks source link

Test case in senderSpec is failing. #213

Closed chirag200666 closed 8 years ago

chirag200666 commented 8 years ago

Because of a recent merging of the pull request (https://github.com/ToothlessGear/node-gcm/pull/212), which fixed few issues in the file, one of the test cases which spans at the lines 125-135 is failing.

eladnava commented 8 years ago

This is the problematic test:

it('should not set strictSSL of req object if the one passed into constructor is not a boolean', function () {
      var options = {
        proxy: 'http://myproxy.com',
        maxSockets: 100,
        timeout: 1000,
        strictSSL: "hi"
      };
      var sender = new Sender('mykey', options);
      var m = new Message({ data: {} });
      sender.sendNoRetry(m, '', function () {});
      expect(args.options.strictSSL).to.be.an('undefined');
    });

It fails because the request option strictSSL does get set to "hi" which is not a boolean.

Currently, we construct the request object parameters like this:

    // Get externally-provided request options passed to gcm.Sender() constructor
    var sender_options = this.options || {};

    // Set request options to default to externally-provided options, and override with our own post_options (allow setting new headers)
    var request_options = _.defaultsDeep(post_options, sender_options);

    // Allow overriding timeout externally, set to default SOCKET_TIMEOUT if not provided
    if (! request_options.timeout) {
        request_options.timeout = Constants.SOCKET_TIMEOUT;
    }

@hypesystem do you think we should add a specific validation check to make sure that strictSSL is passed correctly? Not sure this test is necessary.

hypesystem commented 8 years ago

I think, in this case, the test is actually wrong. If users pass invalid request arguments to us, we should just pass them on and let request handle it the way it normally would.

eladnava commented 8 years ago

@hypesystem @chirag200666 submitted and merged a PR to remove this failing test. Thanks for bringing this up, @chirag200666!