Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: #925 added setOption in case of fileUploadClient #926

Merged
merged 7 commits into from
Feb 4, 2021

Conversation

abossard
Copy link
Contributor

@abossard abossard commented Feb 3, 2021

Checklist

Reference/Link to the issue solved with this PR (if any)

#925

Description of the problem

Pass the options also to the file upload client

Description of the solution

Checked if there is a fileUploadClient in use, and if so, the options are added to it.

@YoDaMa
Copy link
Contributor

YoDaMa commented Feb 3, 2021

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@YoDaMa
Copy link
Contributor

YoDaMa commented Feb 3, 2021

thanks for the contribution @abossard! can you please add a test for this condition?

It would look very similar to the following test in _blob_upload_client_test.js, but it would go inside _device_client_test.js. It looks like the describe block for the setoptions in device client test is at line 463, so you could start there.

  describe('#setOptions', function () {
    /*Tests_SRS_NODE_DEVICE_BLOB_UPLOAD_CLIENT_99_011: [`setOptions` shall set `fileUploadApi` options.]*/
    it('sets file upload API options', function () {
      var fakeOptions = '__FAKE_OPTIONS__';
      var fakeFileUpload = new FakeFileUploadApi();
      var client = new BlobUploadClient(fakeConfig, fakeFileUpload, null);
      client.setOptions(fakeOptions);
      assert.isTrue(fakeFileUpload.setOptions.calledWith(fakeOptions));
    });
  });

@abossard
Copy link
Contributor Author

abossard commented Feb 4, 2021

Hi @YoDaMa thanks for pointing it out. I added the required test.

Copy link
Contributor

@anthonyvercolano anthonyvercolano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@anthonyvercolano
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@anthonyvercolano anthonyvercolano merged commit 9bde57d into Azure:master Feb 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants