Skip to content

Implement fleet provisioning test #501

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

Merged
merged 38 commits into from
Nov 16, 2023
Merged

Implement fleet provisioning test #501

merged 38 commits into from
Nov 16, 2023

Conversation

sfod
Copy link
Contributor

@sfod sfod commented Oct 30, 2023

Issue #, if available:

Description of changes:

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@xiazhvera
Copy link
Contributor

Is it possible to add a junit test like ShadowStateTest instead of having a different set of test script and codes?

@sfod
Copy link
Contributor Author

sfod commented Oct 30, 2023

Is it possible to add a junit test like ShadowStateTest instead of having a different set of test script and codes?

The script is needed to check that fleet provisioning created a thing. And then this script deletes the thing.
Is it possible to do this easily without using boto3?

Copy link
Contributor

@xiazhvera xiazhvera left a comment

Choose a reason for hiding this comment

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

The run_service_test and FleetPorvisioning test are similar to our sample. Could we directly reuse those? I realized there are some changes related to the is_ci flag. However as we planed to remove the is_ci, I feel the two samples would be almost identical in the future.

@sfod
Copy link
Contributor Author

sfod commented Nov 14, 2023

The run_service_test and FleetPorvisioning test are similar to our sample. Could we directly reuse those? I realized there are some changes related to the is_ci flag. However as we planed to remove the is_ci, I feel the two samples would be almost identical in the future.

They're very similar but even in this PR they already different. The fleet provisioning test supports both mqtt3 and mqtt5, to reduce code duplication. But we don't want to use different mqtt versions in one sample, it'll probably mislead users that using both versions is a good practice.

@sfod sfod requested a review from bretambrose November 14, 2023 20:28
@sfod sfod requested a review from xiazhvera November 14, 2023 20:28
Copy link
Contributor

@xiazhvera xiazhvera left a comment

Choose a reason for hiding this comment

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

Debatable: I feel The current approach has a certain weakness that we could not easily add more test.
Currently we only test for the success path, while for a throughout tests, we probably should also cover the failed path and error handling.

@@ -465,12 +465,27 @@ jobs:
run: |
java -version
mvn install -Dmaven.test.skip
- name: Running samples in CI setup
- name: Running samples and service client tests in CI setup
Copy link
Contributor

Choose a reason for hiding this comment

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

As for a test purpose, we should run the tests for all possible platform instead just on ubuntu-latest.

This might not apply to Java here, while for other SDKs, if we use crt-builder to build the SDK, then we could directly use the build.json script to setup the test for each CI build. You can refer to Python SDK for an example: https://github.com/aws/aws-iot-device-sdk-python-v2/blob/fa4f4ad921748d6377827548b2663d1249cbdedb/builder.json#L90

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I enabled service client tests for one platform only, because we already test builder and connection methods on all platforms. But if you think it might be useful to run these tests on multiple platforms, I can place them near DeviceAdvisor tests.

}
return connWrapper;
} catch (Exception ex) {
throw new RuntimeException("Failed to create MQTT311 connection", ex);
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: would the python script reported error if we throw an exception?

Copy link
Contributor Author

@sfod sfod Nov 15, 2023

Choose a reason for hiding this comment

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

Probably, the script won't report it itself, but the exception will be reported by Java. I threw an exception from main for experiment and got the test failing in CI with this message:

Error:  Failed to execute goal org.codehaus.mojo:exec-maven-plugin:3.1.0:java (default-cli) on project FleetProvisioningServiceTest: An exception occurred while executing the Java class. Invalid MQTT version specified: 3 -> [Help 1]

@sfod
Copy link
Contributor Author

sfod commented Nov 15, 2023

Debatable: I feel The current approach has a certain weakness that we could not easily add more test. Currently we only test for the success path, while for a throughout tests, we probably should also cover the failed path and error handling.

Depending on what exactly we want to check, we might have to implement a new test. But maybe it's possible to integrate alternative paths into the existing tests. If you have a specific case for negative scenario in mind, I can add it to this PR.

@sfod sfod requested a review from xiazhvera November 16, 2023 17:15
@sfod sfod merged commit b43b0fd into main Nov 16, 2023
@sfod sfod deleted the add-fleet-provisioning-test branch November 16, 2023 23:08
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.

4 participants