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

Add integration test module for appconfiguration #15582

Conversation

chenrujun
Copy link

@chenrujun chenrujun commented Sep 24, 2020

Add integration test module for appconfiguration

@chenrujun chenrujun requested a review from saragluna September 24, 2020 02:41
@ghost ghost added App Configuration Azure.ApplicationModel.Configuration azure-spring All azure-spring related issues labels Sep 24, 2020
@chenrujun
Copy link
Author

/azp run java - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@chenrujun
Copy link
Author

/azp run java - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@chenrujun chenrujun changed the title Add test module for appconfiguration, and update tests.yml to run int… Add integration test module for appconfiguration Sep 24, 2020
@chenrujun
Copy link
Author

/azp run java - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@chenrujun
Copy link
Author

/azp run java - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@chenrujun
Copy link
Author

/azp run java - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@chenrujun
Copy link
Author

/azp run java - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@chenrujun
Copy link
Author

/azp run java - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@chenrujun
Copy link
Author

/azp run java - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@chenrujun
Copy link
Author

/azp run java - spring - ci

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@chenrujun
Copy link
Author

/azp run java - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@chenrujun
Copy link
Author

/azp run java - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@chenrujun
Copy link
Author

Hi, @mrm9084 .

  • Please review this PR.
  • After this PR merged, could you please add integration test?

@chenrujun
Copy link
Author

/azp run java - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@chenrujun chenrujun self-assigned this Sep 25, 2020
@chenrujun
Copy link
Author

/azp run java - appconfiguration - tests

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@chenrujun
Copy link
Author

Hi, @saragluna .
Please review this PR when you have time. 🙏

@@ -18,6 +18,13 @@
VERSION_UPDATE_ITEMS = 'version_update_items'

config = {
'appconfiguration': {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than this complexity, wouldn't it be better to just have two test modules with the different versions and use the POM to control where the sources are loaded from?

Copy link
Author

Choose a reason for hiding this comment

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

have two test modules

Test code can not be shared in 2 modules.
We just want to write test code in one place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Remember that this complexity will make your build more brittle and harder for others to debug if something goes wrong. If there is a more "Maveny" way to do this with POM configuration then that would be better. Duplicating the tests might also be preferrable.

Copy link
Author

Choose a reason for hiding this comment

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

If there is a more "Maveny" way to do this with POM configuration then that would be better

I agree, but I didn't find that way.

Duplicating the tests might also be preferrable.

If we have many tests, for example, 50 ~ 100 tests, change the pom file is better than keep same code in 2 place. We already discussed inside our team. And spring module and cosmos module already do like this.

.github/CODEOWNERS Outdated Show resolved Hide resolved
Copy link
Member

@saragluna saragluna left a comment

Choose a reason for hiding this comment

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

LGTM.

@chenrujun chenrujun merged commit 7a099f0 into Azure:master Oct 9, 2020
@chenrujun chenrujun deleted the add-integration-test-module-for-appconfiguration branch October 9, 2020 02:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
App Configuration Azure.ApplicationModel.Configuration azure-spring All azure-spring related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants