Additional unit-tests and unit-test adjustment #611
Conversation
…r.json is modifed and new builder adjustments are done
…sting MQTT311 tests, as well as builder options for the environment variable parsing builder action
…why the MQTT5 client creation in native failed
…nit_test_refactor
…me from 1.1 seconds to 1.2
…nit_test_refactor
…ntials everywhere
…nit_test_refactor
…nit_test_refactor
…eally dependent on socket speed
| @@ -1667,6 +1666,8 @@ JNIEXPORT jlong JNICALL Java_software_amazon_awssdk_crt_mqtt5_Mqtt5Client_mqtt5C | |||
| &client_options.host_name, | |||
| false, | |||
| &was_value_set) != AWS_OP_SUCCESS) { | |||
| s_aws_mqtt5_client_log_and_throw_exception( | |||
| env, "MQTT5 client new: Could not get host name from options", AWS_ERROR_INVALID_STATE); | |||
There was a problem hiding this comment.
Trivial: All these exceptions are returning AWS_ERROR_INVALID_STATE which is "An invalid state was encountered." I'm not 100% on how JNI is getting all the various members out from the jobject but wondering out loud if invalid state is the most appropriate error to throw. Or maybe it doesn't matter?
There was a problem hiding this comment.
Maybe AWS_ERROR_INVALID_ARGUMENT argument would be better? It's output is: "An invalid argument was passed to a function." which might be a bit more accurate.
Not sure if I want to change it here since this is mostly testing stuff and I think the bindings return AWS_ERROR_INVALID_STATE already in a lot of places, but I can do this as a follow-up PR.
src/test/java/software/amazon/awssdk/crt/test/CustomKeyOpsTest.java
Outdated
Show resolved
Hide resolved
src/test/java/software/amazon/awssdk/crt/test/IotServiceTest.java
Outdated
Show resolved
Hide resolved
src/test/java/software/amazon/awssdk/crt/test/Mqtt5ClientTest.java
Outdated
Show resolved
Hide resolved
src/test/java/software/amazon/awssdk/crt/test/MqttClientConnectionFixture.java
Outdated
Show resolved
Hide resolved
|
Thanks for taking a look at this and leaving feedback! I'll start incorporating it as soon as I get C++ done |
…orks and the test is extremely flakey even with pauses to allow for IoT Core limits
| @@ -1362,69 +1362,6 @@ public void ConnNegativeID_UC7() { | |||
| } | |||
| } | |||
|
|
|||
| /* Double Client ID disconnect and then reconnect test */ | |||
There was a problem hiding this comment.
Removing the double client ID reconnect test. It wasn't part of the testing plan anyway and it is extremely flakey and inconsistent. We know reconnect works and I don't think it is adding anything to test coverage we don't already have, but it does cause CI to fail (not only here, but in other PRs too).
We can always add it back if there's a need but for now, I'm just going to remove it.
|
Thanks! Merging into |
Description of changes:
Adds new unit tests to check for MQTT311 and MQTT5 connections for:
As well as adjusting existing tests to use environment variables instead of Java properties. It uses a new CRT builder action here to setup the environment variables. Finally, it adjusts Codebuild to also use the CRT builder so everything has the same setup across the board.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.