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

DataService integration test: happy case! #366

Merged
merged 7 commits into from Feb 1, 2018

Conversation

Projects
None yet
3 participants
@xjhe
Copy link
Contributor

commented Jan 30, 2018

Summary

Add some test for the happy case of DS integration test. More detail test and corner cases are already covered in unit tests.
I couldn't test DescribeEnvironmentRevision and StartDeployment because we don't have ListEnvironments yet (why ? we don't need it in our second milestone?)

Also, figure that in DeleteEnvironmentApi, if we don't use the configuration SaveBehavior.CLOBBER.config(), the deletion will fail. But why? I think the default config will still allow us to delete one environment with just one revision.

Implementation details

Testing

./gradlew check
./gradlew clean build

Licensing

This contribution is under the terms of the Apache 2.0 License: Yes

@xjhe xjhe self-assigned this Jan 30, 2018

@xjhe xjhe requested review from wjbuys and wbingli Jan 30, 2018

import java.util.Arrays;
import org.junit.Test;

public class DataServiceIntegrationTest extends DataServiceIntegrationTestBase {

This comment has been minimized.

Copy link
@wbingli

wbingli Jan 31, 2018

Member

Not sure DataServiceIntegrationTest is a good name, I would suggest we move separate each API into its own test class. Even they are just happy path, but we potentially could add more tests, such as exception handling cases. Thoughts?

This comment has been minimized.

Copy link
@xjhe

xjhe Jan 31, 2018

Author Contributor

That makes sense. I can break it down into separate tests for each API, so later people can add the non-happy case tests in, if necessary.

@wjbuys

wjbuys approved these changes Feb 1, 2018

Copy link
Contributor

left a comment

Looks good, although I wonder if it's helpful to have both the e.g. ACCOUNT_ID constants, as well as the default values on DataServiceModelBuilder. Perhaps we can either use e.g. models.getAccountId() in the tests, or set the default account ID on the model builder to ACCOUNT_ID where we construct it?

@xjhe

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2018

Don't know how to directly reply @wjbuys 's review message...so I post a new message here.
Yes I think getting the account ID from the model builder is fine. In our tests we only use this default account ID, so we don't need to customize it

import org.junit.Test;

public class DescribeEnvironmentIntegrationTest extends DataServiceIntegrationTestBase {
private static final String ACCOUNT_ID = "123456789012";

This comment has been minimized.

Copy link
@wbingli

wbingli Feb 1, 2018

Member

Looks the "ACCOUNT_ID", "ENVIRONMENT_NAME" and envionrmentId are pretty much the same for all the test, we most likely could move into the base class. Even some of the test case need different environment or cluster, we can have different name for those.

This comment has been minimized.

Copy link
@xjhe

xjhe Feb 1, 2018

Author Contributor

The ENVIRONMENT_NAME and CLUSTER can be different, but I can use the customized value for them

@xjhe

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2018

The new update removes unnecessary values in account id, environment and cluster. Use most of the default values, unless the values are different from the default.

@wbingli

wbingli approved these changes Feb 1, 2018

Copy link
Member

left a comment

/shipit

@xjhe xjhe merged commit 905306c into blox:dev Feb 1, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.