You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@seata.apache.org by "mehedikhan72 (via GitHub)" <gi...@apache.org> on 2025/03/16 21:22:04 UTC
[PR] test: added unit tests for NacosRegistryServiceImple [incubator-seata]
mehedikhan72 opened a new pull request, #7225:
URL: https://github.com/apache/incubator-seata/pull/7225
Unit tests added for NacosRegistryServiceImplementation
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] test: added unit tests for NacosRegistryServiceImple [incubator-seata]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #7225:
URL: https://github.com/apache/incubator-seata/pull/7225#issuecomment-2729178381
## [Codecov](https://app.codecov.io/gh/apache/incubator-seata/pull/7225?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 53.61%. Comparing base [(`d300796`)](https://app.codecov.io/gh/apache/incubator-seata/commit/d3007963776ad08f88bb14bf58b156abb37cf2fa?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`146e073`)](https://app.codecov.io/gh/apache/incubator-seata/commit/146e07392f9687227c589820b34ac804694d54c0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/incubator-seata/pull/7225?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## 2.x #7225 +/- ##
============================================
- Coverage 53.61% 53.61% -0.01%
+ Complexity 7128 7127 -1
============================================
Files 1171 1171
Lines 41619 41619
Branches 4877 4877
============================================
- Hits 22315 22314 -1
- Misses 17194 17195 +1
Partials 2110 2110
```
[see 7 files with indirect coverage changes](https://app.codecov.io/gh/apache/incubator-seata/pull/7225/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary>🚀 New features to boost your workflow: </summary>
- ❄ [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- 📦 [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] test: added unit tests for NacosRegistryServiceImple [incubator-seata]
Posted by "mehedikhan72 (via GitHub)" <gi...@apache.org>.
mehedikhan72 commented on PR #7225:
URL: https://github.com/apache/incubator-seata/pull/7225#issuecomment-2745545774
Hi @slievrly . Can you take a look? Since these are mock tests, how else would be improve these? The actual functionalities are tightly coupled(Depends on the discovery services) which makes me think these are not good cases for mock tests, since we are mocking the core functionalities.
Please let me know if I'm missing any key points, thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] test: added unit tests for NacosRegistryServiceImple [incubator-seata]
Posted by "slievrly (via GitHub)" <gi...@apache.org>.
slievrly commented on PR #7225:
URL: https://github.com/apache/incubator-seata/pull/7225#issuecomment-2746779595
> Hi @slievrly . Can you take a look? Since these are mock tests, how else would we improve these? The actual functionalities are tightly coupled(Depends on the discovery services) which makes me think these are not good cases for mock tests, since we are mocking the core functionalities.
>
> Please let me know if I'm missing any key points, thanks.
Some third-party configuration registries themselves provide mock server capabilities. If they don't, then we'll consider launching a registry container in the github action pipeline to do real testing. If that's not possible, we'll consider mock test to simulate the registry's behavior to see if Seata behaves as expected.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] test: added unit tests for NacosRegistryServiceImple [incubator-seata]
Posted by "slievrly (via GitHub)" <gi...@apache.org>.
slievrly commented on code in PR #7225:
URL: https://github.com/apache/incubator-seata/pull/7225#discussion_r1999062950
##########
discovery/seata-discovery-nacos/src/test/java/org/apache/seata/discovery/registry/nacos/NacosRegistryServiceImplTest.java:
##########
@@ -40,5 +46,34 @@ public void testGetConfigProperties() throws Exception {
Assertions.assertThat(properties.getProperty("contextPath")).isEqualTo("/bar");
}
+ @Test
+ public void testRegister() throws Exception {
+ RegistryService registryService = mock(NacosRegistryServiceImpl.class);
+ InetSocketAddress inetSocketAddress = new InetSocketAddress("127.0.0.1", 8091);
+ registryService.register(inetSocketAddress);
+ verify(registryService).register(inetSocketAddress);
Review Comment:
Aside from the verify method being called, can we test more fine-grained expected results?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] test: added unit tests for NacosRegistryServiceImple [incubator-seata]
Posted by "slievrly (via GitHub)" <gi...@apache.org>.
slievrly closed pull request #7225: test: added unit tests for NacosRegistryServiceImple
URL: https://github.com/apache/incubator-seata/pull/7225
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] test: added unit tests for NacosRegistryServiceImple [incubator-seata]
Posted by "funky-eyes (via GitHub)" <gi...@apache.org>.
funky-eyes commented on PR #7225:
URL: https://github.com/apache/incubator-seata/pull/7225#issuecomment-2746953255
https://github.com/apache/incubator-seata/blob/2.x/.github/workflows/build.yml#L38
I don't think mocking is necessary because the Nacos container is already running in the CI environment on Linux. Therefore, we can directly perform the related tests, as long as we add a condition to run only in the Linux environment
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] test: added unit tests for NacosRegistryServiceImple [incubator-seata]
Posted by "mehedikhan72 (via GitHub)" <gi...@apache.org>.
mehedikhan72 commented on code in PR #7225:
URL: https://github.com/apache/incubator-seata/pull/7225#discussion_r1999453253
##########
discovery/seata-discovery-nacos/src/test/java/org/apache/seata/discovery/registry/nacos/NacosRegistryServiceImplTest.java:
##########
@@ -40,5 +46,34 @@ public void testGetConfigProperties() throws Exception {
Assertions.assertThat(properties.getProperty("contextPath")).isEqualTo("/bar");
}
+ @Test
+ public void testRegister() throws Exception {
+ RegistryService registryService = mock(NacosRegistryServiceImpl.class);
+ InetSocketAddress inetSocketAddress = new InetSocketAddress("127.0.0.1", 8091);
+ registryService.register(inetSocketAddress);
+ verify(registryService).register(inetSocketAddress);
Review Comment:
@slievrly I'm working on it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org
Re: [PR] test: added unit tests for NacosRegistryServiceImple [incubator-seata]
Posted by "slievrly (via GitHub)" <gi...@apache.org>.
slievrly commented on PR #7225:
URL: https://github.com/apache/incubator-seata/pull/7225#issuecomment-3414043778
Long time no progress, close it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@seata.apache.org
For additional commands, e-mail: notifications-help@seata.apache.org