refactor(e2e): fix one timeout test and disable another #2121
Conversation
timtay-microsoft
requested review from
abhipsaMisra,
azabbasi,
barustum,
drwill-ms,
jamdavi and
vinagesh
as code owners
| // The provisioning transport layer will typically throw an OperationCanceledException, but some clock issues | ||
| // may cause this test to throw the OperationCanceledException nested in a ProvisioningTransportException instead. | ||
| return; // expected exception was thrown, so exit the test | ||
| } |
There was a problem hiding this comment.
We have a check on the cancellation token that we create from a timespan as the first line in our provisioning transport handler for HTTP, but for some reason, it can sometimes pass that check even with Timespan.Zero. The next check on the timespan will throw an OperationCanceledException, but nested in a ProvisioningTransportException.
There was a problem hiding this comment.
this is interesting, do you have a link to the code that you're referring to?
There was a problem hiding this comment.
There was a problem hiding this comment.
Can we add a check in our SDK to ensure TimeSpan.Zero always throws an OperationCancelledException. The reason I say this is, while these layered checks might be ok for our tests, they'll make an uncomfortable usage experience for our customers.
There was a problem hiding this comment.
I can, but do you expect users to call these APIs with a timespan of 0?
There was a problem hiding this comment.
I get that a user wouldn't want to make the same catch statements that I made for this test, but adding a check for Timespan.Zero seems like the wrong fix for that issue. I'd rather the AMQP library itself be fixed to be more consistent with handling it
There was a problem hiding this comment.
I can, but do you expect users to call these APIs with a timespan of 0?
Good point, but people do weird things, it is better for us to exhibit the "correct" behavior irrespective of the crazy inputs! If this is an issue with the http client, let's reach out to the team and get it fixed. In the meanwhile we can add checks in our code that account for their bug.
Btw, what scenario are we testing with a timespan of 0? If we're simply testing our SDK logic, would this be better as a unit test instead?
I'd rather the AMQP library itself be fixed to be more consistent with handling it
This is the http test. The amqp one is the one below. :)
|
|
||
| throw new AssertFailedException("Expected an OperationCanceledException to be thrown since the timeout was set to TimeSpan.Zero"); | ||
| } | ||
|
|
||
| [Ignore] // AMQP library itself is unreliable at returning the expected error code, and sometimes throws an ObjectDisposedException |
There was a problem hiding this comment.
There is some threading at play since the AMQP library sets a timer based on the timespan we provide. Because the library does not behave consistently, I'm proposing we disable the test for now
There was a problem hiding this comment.
is the plan to disable this test altogether or do we want to investigate this issue further?
There was a problem hiding this comment.
meaning, this being temporary so that our gates pass :)
There was a problem hiding this comment.
We can take this up as an issue with the AMQP library we use, but for now it should be disabled since it is flakey
There was a problem hiding this comment.
Do you think marking it as flaky would be better than disabling it? (Flaky tests won't run on PR gates.)
There was a problem hiding this comment.
I'll defer to you on this one. I see this test as safe to disable since we likely won't mess up the timespan pass-down logic, but if you don't mind seeing the test fail occasionally on the nightly build then I'll mark it as flakey instead
There was a problem hiding this comment.
If we're waiting for a fix from the amqp library let us create an issue on their repo, tag it here and track it on our board. I'm afraid ignored tests will just get lost somewhere in the backlog and we won't look at it unless something else breaks.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
| // The provisioning transport layer will typically throw an OperationCanceledException, but some clock issues | ||
| // may cause this test to throw the OperationCanceledException nested in a ProvisioningTransportException instead. | ||
| return; // expected exception was thrown, so exit the test | ||
| } |
There was a problem hiding this comment.
I can, but do you expect users to call these APIs with a timespan of 0?
Good point, but people do weird things, it is better for us to exhibit the "correct" behavior irrespective of the crazy inputs! If this is an issue with the http client, let's reach out to the team and get it fixed. In the meanwhile we can add checks in our code that account for their bug.
Btw, what scenario are we testing with a timespan of 0? If we're simply testing our SDK logic, would this be better as a unit test instead?
I'd rather the AMQP library itself be fixed to be more consistent with handling it
This is the http test. The amqp one is the one below. :)
|
|
||
| throw new AssertFailedException("Expected an OperationCanceledException to be thrown since the timeout was set to TimeSpan.Zero"); | ||
| } | ||
|
|
||
| [Ignore] // AMQP library itself is unreliable at returning the expected error code, and sometimes throws an ObjectDisposedException |
There was a problem hiding this comment.
If we're waiting for a fix from the amqp library let us create an issue on their repo, tag it here and track it on our board. I'm afraid ignored tests will just get lost somewhere in the backlog and we won't look at it unless something else breaks.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
* feat(shared): Add common resources for convention-based operations * feat(iot-device): Add support for convention-based telemetry operation * feat(iot-device): Add support for convention-based command operations * * feat(iot-device): Add support for convention-based properties operations * feat(iot-device): Add support for convention-based properties operations Co-authored-by: James Davis ⛺️🏔 <jamdavi@microsoft.com> * feat(e2e-tests): Add telemetry E2E tests * feat(e2e-tests): Add command E2E tests * fix(iot-device): Updating client property collection to handle no convention * feat(samples): Add thermostat and temperature controller sample * fix(doc, samples): Update API design doc and move SystemTextJson helper to samples * fix(iot-device): Separate out root-level and component-level property addition operations * feat(tests): Add unit tests for ClientPropertyCollection feat(tests): Add unit tests for ClientPropertyCollection Co-authored-by: Abhipsa Misra <abhipsa.misra@microsoft.com> * feat(e2e-tests): Add properties E2E tests Co-authored-by: Abhipsa Misra <abhipsa.misra@microsoft.com> * feat(e2e-tests): Add fault injection tests for properties operations (#2001) * fix(iot-device, shared, samples): Rename StatusCodes to CommonClientResponseCodes and add a comment to highlight ClientOptions behavior * fix(iot-device): Fix enumerator implementation to return key-value pairs * fix(iot-device): Make ClientOptions.PayloadConvention readonly * fix(shared): Fix from merge conflict * fix(samples): Update the pnp samples readme (#2025) * Rename E2ETests.Configuration to TestConfiguration to avoid SDK type name conflict * Revert "Rename E2ETests.Configuration to TestConfiguration to avoid SDK type name conflict" This reverts commit 75034e5. * Rename E2ETests.Configuration to TestConfiguration to avoid SDK type name conflict * feat(service): add support for DeviceScope to import/export job * fix(iot-device): Update IoT Hub CONNACK timeout to be 60 seconds * feat(provisioning-device, prov-amqp, prov-mqtt, prov-https): Add support for timespan timeouts to provisioning device client (#2041) As discussed in #2036, AMQP provisioning device clients have no way to configure the timeout given to the AMQP library for operations like opening links. This adds overloads to the existing provisioning device client's registerAsync methods that allow for users to configure this timespan timeout. * fix(iot-device): Update doc comments for MqttTransportSettings * Add documentation to repo about which platforms this SDK supports (#2046) fixes #2032 * readme: Update readme to have LTS patch reference * doc(readme): Updated readme with details on support for device streaming feature. (#2054) * fix(iot-device): Update the amqp layer to inspect the inner exception for an amqp error code (#2053) * refactor(samples): Move preview samples to samples repository (#2057) * fix(e2e-tests): Fix file renaming Configutaion to TestConfiguration * fix(iot-device): Update connection string validation param * Update team codeowners * fix(e2e-tests): Update resource generation script to generate resources for samples * initial changeS (#2060) Co-authored-by: Sindhu Nagesh <vinagesh@microsoft.com> * Adding readme to explain authentication, retry and timeouts in DeviceClient (#2096) * refactor(iot-device): Rename property add method * Last of error codes documentation (#2110) (#2074) refactor(iot-service): Add API example for DeviceAlreadyExists error (#2101) fix(iot-service): Hide unreferenced error code (#2094) a few misc errors (#2092) * a few misc errors * fixup Document ArgumentNull (#2091) Document InvalidOperation and ArgumentInvalid (#2089) Updating exception details for IotHubThrottledException (#2088) refactor(iot-service): Add comments for service returned error codes (#2083) Fix build error (#2087) Updating exception details for IotHubSuspendedException (#2086) Updating exception details for IotHubCommunicationException (#2085) Fix InvalidProtocolVersion documentation. (#2084) More codes doc'd and code cleanup (#2082) * More codes doc'd, code clean up * Update license * Random is static * License header * More codes doc'd, code clean up More updates to exceptions thrown (#2081) More error code doc comments Remove ref to ExponentialBackoff [Error Codes] Update ThrottleBacklogLimitExceeded error code (#2078) [Error Codes] Update PreconditionFailed error description (#2077) * [Error Codes] Update PreconditionFailed error description * Remove space [Error Codes] Updated MessageTooLarge (#2073) * [Error Codes] Updated MessageTooLarge * Removed en-us where seen * Remove en-us * REmove en-us [Error Codes] Updating UnauthorizedException (#2072) * [Error Codes] Updating UnauthorizedException * Remove en-us * Remove en-us * Update UnauthorizedException.cs * Update UnauthorizedException.cs [Error Codes] Updated ServerErrorException (#2071) * [Error Codes] Updated ServerErrorException * Update ServerErrorException.cs * Update ServerErrorException.cs * Update ServerErrorException.cs * Update ServerErrorException.cs * Update ServerErrorException.cs [Error Codes] Updated QuotaExceededException (#2070) * Updated QuotaExceededException classes * Update QuotaExceededException.cs * Update QuotaExceededException.cs Updated ServerBusyException (#2069) [Error Codes] Update PartitionNotFound error code (#2075) * [Error Codes] Update PartitionNotFound error code * remove double lines * remove double lines Obsolete error codes that are not throw by the service (#2079) Fix deprecation messages. Notes for and deprecation of BlobContainerValidationError, BulkRegistryOperationFailure, and JobQuotaExceeded Document errors and remove unreferenced file * Rename readme per style and update reference (#2113) * * fix(iot-device): TryGetValue methods should check for the component identifier when applicable and should not throw any exceptions (#2111) * refactor(iot-device): Separate client reported properties into a separate accessor * fix(iot-device): Add checks to verify that client property update response has version and requestId (#2115) * refactor(iot-service) Make all clients mockable (#2117) * refactor(job-client): Make job client easy to mock (#1875) * refactor(service-client): Make service client easy to mock (#1878) * refactor(registry-manager): Make registry manager mockable (#1881) * refactor(digital-twin-client): Make the DigitalTwin client mockable. (#1892) * refactor(iot-service): Move direct method request file out of JobClient folder into root * refactor(prov-amqp, prov-mqtt, prov-http): Fix timeout tests failing occasionally (#2121) * fix(tools): Fix readme for ConsoleEventListener usage (#2123) * fix(tools): Fix readme for ConsoleEventListener usage * doc(all): Add documentation for the upcoming service TLS changes for user awareness (#2112) * fix(doc): Fix typo is readme * Remove barustum from CODEOWNERS (#2126) * refactor(ado): Update area paths for analyzers pipeline (#2136) * refactor(tools-logs): Update log capture script to be configurable and use powershell (#2141) * refactor(iot-device, prov-mqtt): Target new DotNetty version when not net451 (#2129) * refactor(doc): Add additional comments to log capture readme * Bump versions for 2021-08-12 release (#2143) * fix(readme): Update the LTS support dates readme (#2147) * fix(e2e): get values from create call to avoid timing issues * refactor(iot-device): Update the internal method names as per functionality * refactor(e2e): Remove unnecessary value in keyvault from setup file (#2153) * fix(e2e): Fix e2e setup script to choose the correct dps endpoint based on region of the resource (#2134) * fix(readme): Update readme to specify preview strategy * Removing vinagesh from codebase owners list * feat(iot-device): Add convenience method for acknowledging writable property update requests * fix(iot-device): Fix params passed into ObjectDisposedException * fix(githib): Update github issues template * Add support for .NET 5 (#2169) * feat(all): Add support for .NET 5.0 * refactor(all): Update APIs to follow .NET 5.0 patterns * refactor(e2e-tests): Update E2E tests to follow .NET 5.0 patterns * refactor(all): Simplify disposal of IDisposable X509Certificate * fix(vsts): Update the location for log analytics workspace resource * refactor(e2e-tests): Dispose IDisposable types * fix(e2e-tests): Dispose IDisposable types created - hmac * fix(iot-device): Update PayloadCollection to accept dictionary of values (#2171) * Wrong reference to Device Streaming Sample (#2173) * Fix link to Device Streaming Sample * Update iothub/service/samples/readme.md Co-authored-by: David R. Williamson <drwill@microsoft.com> * fix(dps-service): Remove client side validation of x509 CA references format (#2172) * Add Andy to the repos (#2179) * Default Branch update: Update master branch to main. (#2183) * Update master branch to main. * Update horton-e2e.yaml * fix(iot-serv): Fix bug where device scope and parent scopes set to device weren't used in bulk add operations (#2189) #2184 * fix(iot-svc): Cleanup and deprecation warning of code in CryptoKeyGenerator (#2187) * fix(iot-svc): Add support for GeneratePassword to other targets * Don't add net472 support, but keep code cleanup * Add deprecated attribute * Fix test references by making a local copy for tests * Add XML docs for exceptions that can be thrown by ModuleClient.SendEventAsync and DeviceClient.SendEventAsync (#2178) * Add XML docs for exceptions that can be thrown by ModuleClient.SendEventAsync and DeviceClient.SendEventAsync * Add more XML docs for exceptions for ModuleClient.SendEventAsync and DeviceClient.SendEventAsync * Update iothub/device/src/DeviceClient.cs Co-authored-by: David R. Williamson <drwill@microsoft.com> * Add using for exceptions Co-authored-by: David R. Williamson <drwill@microsoft.com> * refactor(iot-device): Merge flows for twin and client property operations (#2180) * fix(e2e-tests): Update E2E tests to initialize and dispose resources correctly * fix Co-authored-by: James Davis ⛺️🏔 <jamdavi@microsoft.com> Co-authored-by: jamdavi <73593426+jamdavi@users.noreply.github.com> Co-authored-by: David R. Williamson <drwill@microsoft.com> Co-authored-by: timtay-microsoft <timtay@microsoft.com> Co-authored-by: Sindhu Nagesh <vinagesh@microsoft.com> Co-authored-by: bikamani <41314966+bikamani@users.noreply.github.com> Co-authored-by: Bommas <5359504+scb01@users.noreply.github.com> Co-authored-by: Basel Rustum <barustum@microsoft.com> Co-authored-by: Roman Marusyk <Marusyk@users.noreply.github.com> Co-authored-by: Azad Abbasi <azabbasi@microsoft.com>
No description provided.