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.

Sign up for GitHub

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

Jump to bottom

Improve code coverage DevicesClient V2 #3073

Merged
merged 15 commits into from
Jan 17, 2023
Merged

Improve code coverage DevicesClient V2 #3073

merged 15 commits into from
Jan 17, 2023

Conversation

schoims
Copy link
Contributor

@schoims schoims commented Jan 13, 2023 •
edited
Loading

Added unit tests for

  • ConfigurationClient
  • ModulesClient
  • MessageClient
  • MessageFeedbackProcessorClient
  • FileUploadNotificationProcessorClient

schoims
schoims commented Jan 16, 2023
View reviewed changes
iothub/service/src/Amqp/AmqpConnectionHandler.cs
@@ -66,7 +72,7 @@ internal class AmqpConnectionHandler : IDisposable
/// Returns false otherwise.
/// </summary>
/// <returns>True if this connection, its sessions and its sessions' links are all open. False otherwise.</returns>
internal bool IsOpen => _connection != null
internal virtual bool IsOpen => _connection != null
&& _connection.State == AmqpObjectState.Opened
Copy link
Contributor Author

@schoims schoims Jan 16, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Made IsOpen virtual to override the implementation for unit testing.

Copy link
Member

Choose a reason for hiding this comment

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

I'd recommend adding a comment explaining this, otherwise it might be confusing as to why a class that isn't being inherited from in the sdk has virtual methods.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure!

schoims
schoims commented Jan 16, 2023
View reviewed changes
iothub/service/src/Amqp/AmqpConnectionHandler.cs
@@ -78,7 +84,7 @@ internal class AmqpConnectionHandler : IDisposable
/// then opening all the required sessions and links.
/// </summary>
/// <param name="cancellationToken">The cancellation token.</param>
internal async Task OpenAsync(CancellationToken cancellationToken)
internal virtual async Task OpenAsync(CancellationToken cancellationToken)
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the method virtual to override the implementation for unit testing.

schoims
schoims commented Jan 16, 2023
View reviewed changes
iothub/service/src/Amqp/AmqpConnectionHandler.cs
@@ -187,7 +193,7 @@ internal async Task OpenAsync(CancellationToken cancellationToken)
/// Closes the AMQP connection. This closes all the open links and sessions prior to closing the connection.
/// </summary>
/// <param name="cancellationToken">The cancellation token.</param>
internal async Task CloseAsync(CancellationToken cancellationToken)
internal virtual async Task CloseAsync(CancellationToken cancellationToken)
{
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Made the method virtual to override the implementation for unit testing.

schoims
schoims commented Jan 16, 2023
View reviewed changes
iothub/service/src/Feedback/MessageFeedbackProcessorClient.cs
_internalRetryHandler = retryHandler;
_amqpConnection = amqpConnection;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Created another constructor to mock AmqpConnectionHandler for unit testing

@schoims schoims marked this pull request as ready for review January 16, 2023 17:58
schoims
schoims commented Jan 16, 2023
View reviewed changes
iothub/service/tests/FileUpload/FileUploadNotificationProcessorClientTests.cs

mockAmqpConnectionHandler
.Setup(x => x.OpenAsync(It.IsAny<CancellationToken>()))
.Returns(Task.CompletedTask);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Using the new constructor for mocking AmqpConnectionHandler

@drwill-ms drwill-ms changed the title Improve code coverage Devices V2 Improve code coverage DevicesClient V2 Jan 17, 2023
@schoims schoims merged commit 0d2ca58 into choisophia/improve-code-coverage-devices Jan 17, 2023
@schoims schoims deleted the choisophia/improve-code-coverage-devices-sophia branch January 17, 2023 22:32
schoims added a commit that referenced this pull request Jan 18, 2023
@schoims
Improve code coverage DevicesClient V2 (#3073)
025a5bd 
* Add more unit tests

* Address comments

* Create new tests

* Work in progress

* Work in progress

* Work in progress

* Add more unit tests

* Make methods virtual and write more unit test cases

* Add more test

* Add comment

* Rename test case

* remove extra line

* remove extra line

* Address comments

* Add comments
patilsnr pushed a commit that referenced this pull request Jan 23, 2023
@schoims @patilsnr
Improve code coverage DevicesClient V2 (#3073)
9799471 
* Add more unit tests

* Address comments

* Create new tests

* Work in progress

* Work in progress

* Work in progress

* Add more unit tests

* Make methods virtual and write more unit test cases

* Add more test

* Add comment

* Rename test case

* remove extra line

* remove extra line

* Address comments

* Add comments
patilsnr added a commit that referenced this pull request Mar 1, 2023
@patilsnr @schoims
Add unit test coverage to v2 service client (#3137)
4a77f6d 
* Improve unit test coverages for Devices V2 (#3046)

* Add test cases

* Add more tests

* Add more unit tests

* Address comments

* Address comments

* Address comments

* Add unit tests for ConfigurationsClient

* remove unused import statements

* Add more test cases and address comments

* Add a simple message unit test

* Npatilsen/improve unit test coverage (#3047)

* connection string tests -- connection string to 90+, connection string parser to 85+, sasbuilder to 100

* added clientTwinProperties test

* TwinsClient tests up to 92% and 76%

* dmcr tests

* dcmr part 2

* direct method service request coverage

* direct method service request coverage p2

* exponentialretrypolicy

* address comments

* Improve code coverage DevicesClient V2 (#3073)

* Add more unit tests

* Address comments

* Create new tests

* Work in progress

* Work in progress

* Work in progress

* Add more unit tests

* Make methods virtual and write more unit test cases

* Add more test

* Add comment

* Rename test case

* remove extra line

* remove extra line

* Address comments

* Add comments

* Npatilsen improve code coverage 2 (#3063)

* iothubserviceclient tests started

* iothubservice client part 2

* fixeddelayretrypolicy

* extra fixedretry and incrementalretry policy tests

* connection properties and sas properties tests

* addressed comments

* reformatted logic for connection properties hostname

* more comments

* Fix error (#3078)

* added serialized client twins to http response (#3079)

* Remove invalid test

* Add new unit tests in V2 Devices to improve coverage (#3089)

* Add tests for IotHubTokenCredentialProperties and deserialization

* Rename tests

* work in progress

* Work in progress

* Work in progress

* Add more tests

* Added more tests

* Fix broken test

* Make changes

* Make changes

* Address comments 1

* Address comments

* Address comments

* Address comments

* Update test method

* minor change

* add a test

* address comments

* address comments

* Add additional assertion for validation

* address comment

* fix typo

* address comments

* remove extra assertions

* address comments

* Improve code coverage for AMQP Devices V2 (#3099)

* Add tests for IotHubTokenCredentialProperties and deserialization

* Work in progress

* Work in progress

* Add more tests

* Added more tests

* Fix broken test

* Make changes

* Address comments 1

* Address comments

* Address comments

* minor change

* address comments

* address comments

* Add additional assertion for validation

* remove extra assertions

* work in progress

* test

* work in progress

* work in progress

* work in progress

* Work in progress

* Work in progress

* Fix

* Fix

* Fix

* Add more tests

* Small updates

* remove duplicate test file

* minor fixes

* minor change

* Remove unnecessary test case

* Address comments

* address comments

* address pr comments

* added subclient test coverage (serviceClient) (#3088)

* rebase and digitalTwinsClient tests

* digitalTwinsClient bugs fixed

* query client tests outlines

* query test attempts

* scheduled jobs client and retry policy base fix

* debugged query client tests

* addressed comments p1

* addressed comments p2

* fixes part 2

* fixed retry policy base

* fixes p3

* additional comments

* made location more realistic

* better test names

* final comments addressed

* Npatilsen/misc files missing tests (#3118)

* rebase and digitalTwinsClient tests

* digitalTwinsClient bugs fixed

* query client tests outlines

* query test attempts

* scheduled jobs client and retry policy base fix

* debugged query client tests

* addressed comments p1

* addressed comments p2

* fixes part 2

* fixed retry policy base

* new classes p1

* new classes p1

* merge conflict fixes

* merge conflict fixes p2

* retry policy base revert

* serialize jobrequests

* scheduledjobsoptions serialization

* queryresponse

* addressed comments

* addressed comments

* nit comments

* rebase onto previews/v2

* Delete haproxy.pem

* unsealed amqp classes to make mockable

* feedback and error fixes

* address error message

* compiler warning fix

* compiler warning fix p2

* fix failing tests

---------

Co-authored-by: Sophia Ji Who Choi <[email protected]>
Co-authored-by: Sophia Choi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patilsnr patilsnr patilsnr approved these changes

@drwill-ms drwill-ms drwill-ms approved these changes

@timtay-microsoft timtay-microsoft Awaiting requested review from timtay-microsoft

@abhipsaMisra abhipsaMisra Awaiting requested review from abhipsaMisra

@andyk-ms andyk-ms Awaiting requested review from andyk-ms

@brycewang-microsoft brycewang-microsoft Awaiting requested review from brycewang-microsoft

@tmahmood-microsoft tmahmood-microsoft Awaiting requested review from tmahmood-microsoft

@ngastelum-ms ngastelum-ms Awaiting requested review from ngastelum-ms

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@schoims @drwill-ms @abhipsaMisra @patilsnr