[ISSUE #4602] when wechat send message api response errcode is not zero, the wechat sink connector does not throw IllegalAccessException #4603

wizardzhang · 2023-12-05T05:49:25Z

Motivation

fix bug when wecaht api response fail

Modifications

rename org.apache.eventmesh.connector.wechat.sink.connector.TemplateMessageResponse#errocode
to org.apache.eventmesh.connector.wechat.sink.connector.TemplateMessageResponse#errcode
add abnormal test case

…e.errcode

pandaapo · 2023-12-05T09:18:16Z

.../src/main/java/org/apache/eventmesh/connector/wechat/sink/connector/WeChatSinkConnector.java

@@ -166,9 +166,9 @@ private void sendMessage(ConnectRecord record) {
                throw new IOException("message response is null.");
            }

-            if (messageResponse.getErrocode() != 0) {
+            if (messageResponse.getErrcode() != 0) {
                throw new IllegalAccessException(String.format("Send message to weCom error! errorCode=%s, errorMessage=%s",


Please change "weCom" to "wechat".

ok, thank for your reveiw,

pandaapo · 2023-12-05T09:25:00Z

.../test/java/org/apache/eventmesh/connector/wechat/sink/connector/WeChatSinkConnectorTest.java

        weChatSinkConnector.put(records);
-        verify(okHttpClient, times(times + 1)).newCall(any(Request.class));
+        verify(okHttpClient, times(2)).newCall(any(Request.class));


Is it better to use JUnit's assertThrows() to assert exceptions at this point?

这时候使用JUnit的assertThrows()断言异常是不是更好一点？

weChatSinkConnector.sendMessage() method throw IllegalAccessException then weChatSinkConnector.put() catch the exception, i want to test weChatSinkConnector.sendMessage() ,but it is a private method ,i used reflect api to invoke the weChatSinkConnector.sendMessage() and then throw InvocationTargetException, IllegalAccessException has been transfer to InvocationTargetException, so the code will like this

i think it looks a little strange

WeChatSinkConnector.sendMessage()方法抛出IllegalAccessException然后weChatSinkConnector.put()捕捉异常，我想测试weChatSinkConnector.sendMessage()，但它是一个私有方法，我使用反射API调用weChatSinkConnector.sendMessage()，然后抛出InvocationTargetException，IllegalAccessException已经传递给InvocationTargetException，所以代码将如下所示。
我觉得它看起来有点奇怪

any idea for test private method， i konw powermock can test private method, it is a good idea to use it? @pandaapo

i used reflect api to invoke the weChatSinkConnector.sendMessage() and then throw InvocationTargetException, IllegalAccessException has been transfer to InvocationTargetException, ... i think it looks a little strange

It may seem a bit strange, but I don't have a better idea at the moment. I personally disagree with PowerMockito, as it is not as restrained as Mockito, but it's just my personal opinion. You can choose to use reflection, PowerMockito, or directly test abnormal put() to indirectly test abnormal sendMessage(), or write a public sendMessageOnlyForTest() that calls sendMessage() in WeChatSinkConnector and test this method.

确实这样会有点奇怪，但是我目前也没有更好的主意。对于PowerMockito，我个人不太认同，它不像Mockito更加克制，但只是我个人观点。您可以自己选择使用反射，或者PowerMockito，或者直接测试put()异常情况来间接测试sendMessage()异常情况，或者写个调用sendMessage()的公开的sendMessageOnlyForTest()在WeChatSinkConnector中，然后测试这个方法。

ok, i choose to use JUnit's assertThrows()

pandaapo

You can add this Sink Connector to the list in this document. https://github.com/apache/eventmesh/tree/master/eventmesh-connectors#connector-status

wizardzhang · 2023-12-05T13:38:16Z

You can add this Sink Connector to the list in this document. https://github.com/apache/eventmesh/tree/master/eventmesh-connectors#connector-status

already done, please review

pandaapo · 2023-12-05T14:05:57Z

You can add this Sink Connector to the list in this document. https://github.com/apache/eventmesh/tree/master/eventmesh-connectors#connector-status

already done, please review

I saw that you did this work in another PR. You can close that PR and make modifications directly in this PR.

…erver again to fix org.mockito.exceptions.misusing.UnnecessaryStubbingException

pandaapo · 2023-12-05T23:12:56Z

.../test/java/org/apache/eventmesh/connector/wechat/sink/connector/WeChatSinkConnectorTest.java

+        weChatSinkConnector.put(records);
+        verify(okHttpClient, times(2)).newCall(any(Request.class));
+
+        WeChatSinkConnector.ACCESS_TOKEN_CACHE.invalidate(WeChatSinkConnector.ACCESS_TOKEN_CACHE_KEY);


I am thinking about the necessity of this line of code. It seems that you did not actually interact with WeChat in your test, so you should not be using this token.

我在思考这行代码的必要性。您的测试中好像并不会真的与wechat交互，应该用不到该token吧。

first u should use app id and app secret to get a token, then use token to send message. so i mock a get token response and store the token in cache, in testSendMessageToWeChat() method, the mock token repsone is used, and then in testSendMessageToWeChatAbnormally() method, the mock token repsone will not be used, because token has been stored in cache and then will throw a org.mockito.exceptions.misusing.UnnecessaryStubbingException. the test result like this

testSendMessageToWeChat() passed, testSendMessageToWeChatAbnormally() failed

pandaapo

Finally, are you willing to write a document for your connector (#4601 (comment)) to facilitate user understanding and use. If you are willing, you can do it in this PR or in a new PR in the future.

最后，请问是否愿意给您的这个Connector写一下文档（#4601 (comment) ），方便用户理解和使用。如果您愿意，可以在该PR中做，也可以以后在新的PR中做。

mxsm

LGTM

codecov · 2023-12-06T03:49:08Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (2a46b82) 16.93% compared to head (133a64b) 16.95%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@             Coverage Diff              @@
##             master    #4603      +/-   ##
============================================
+ Coverage     16.93%   16.95%   +0.01%     
- Complexity     1693     1695       +2     
============================================
  Files           788      788              
  Lines         29420    29420              
  Branches       2539     2539              
============================================
+ Hits           4983     4987       +4     
+ Misses        23972    23969       -3     
+ Partials        465      464       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

wizardzhang · 2023-12-06T07:56:00Z

Finally, are you willing to write a document for your connector (#4601 (comment)) to facilitate user understanding and use. If you are willing, you can do it in this PR or in a new PR in the future.

最后，请问是否愿意给您的这个Connector写一下文档（#4601 (comment) ），方便用户理解和使用。如果您愿意，可以在该PR中做，也可以以后在新的PR中做。

i prefer in a new PR to do this

Pil0tXia

LGTM

wizardzhang · 2023-12-08T01:13:01Z

@pandaapo hello, i found some commit in this PR used wrong email, so i want to close this PR, an create a new one

张忠炜 added 2 commits December 5, 2023 10:45

rename the TemplateMessageResponse.errocode to TemplateMessageRespons…

ca2b66d

…e.errcode

add unit test for abnormal case

7a8d2e8

wizardzhang changed the title ~~[ISSUE #4602]~~ [ISSUE #4602] fix bug when wecaht api response fail Dec 5, 2023

pandaapo reviewed Dec 5, 2023

View reviewed changes

pandaapo changed the title ~~[ISSUE #4602] fix bug when wecaht api response fail~~ [ISSUE #4602] when wechat send message api response errcode is not zero, the wechat sink connector does not throw IllegalAccessException Dec 5, 2023

pandaapo reviewed Dec 5, 2023

View reviewed changes

change the weCom to WeChat

7cc713b

wizardzhang added 3 commits December 5, 2023 22:42

add WeChat sink connector module in eventmesh-connectors/README.md

0ab75eb

use JUnit's assertThrows() to assert exceptions

0bf57f0

invalid token,testSendMessageToWeChatAbnormally will get token from s…

133a64b

…erver again to fix org.mockito.exceptions.misusing.UnnecessaryStubbingException

pandaapo reviewed Dec 5, 2023

View reviewed changes

mxsm approved these changes Dec 6, 2023

View reviewed changes

Pil0tXia approved these changes Dec 6, 2023

View reviewed changes

wizardzhang requested a review from pandaapo December 7, 2023 01:24

wizardzhang closed this Dec 8, 2023

pandaapo mentioned this pull request Dec 8, 2023

[ISSUE #4602] when wechat send message api response errcode is not zero, the wechat sink connector does not throw IllegalAccessException #4627

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ISSUE #4602] when wechat send message api response errcode is not zero, the wechat sink connector does not throw IllegalAccessException #4603

[ISSUE #4602] when wechat send message api response errcode is not zero, the wechat sink connector does not throw IllegalAccessException #4603

wizardzhang commented Dec 5, 2023

pandaapo Dec 5, 2023

wizardzhang Dec 5, 2023

pandaapo Dec 5, 2023

wizardzhang Dec 5, 2023

wizardzhang Dec 5, 2023

pandaapo Dec 5, 2023 •

edited

Loading

wizardzhang Dec 5, 2023

pandaapo left a comment

wizardzhang commented Dec 5, 2023

pandaapo commented Dec 5, 2023 •

edited

Loading

pandaapo Dec 5, 2023

wizardzhang Dec 6, 2023 •

edited

Loading

pandaapo left a comment

mxsm left a comment

codecov bot commented Dec 6, 2023 •

edited

Loading

wizardzhang commented Dec 6, 2023

Pil0tXia left a comment

wizardzhang commented Dec 8, 2023

[ISSUE #4602] when wechat send message api response errcode is not zero, the wechat sink connector does not throw IllegalAccessException #4603

[ISSUE #4602] when wechat send message api response errcode is not zero, the wechat sink connector does not throw IllegalAccessException #4603

Conversation

wizardzhang commented Dec 5, 2023

Motivation

Modifications

pandaapo Dec 5, 2023

Choose a reason for hiding this comment

wizardzhang Dec 5, 2023

Choose a reason for hiding this comment

pandaapo Dec 5, 2023

Choose a reason for hiding this comment

wizardzhang Dec 5, 2023

Choose a reason for hiding this comment

wizardzhang Dec 5, 2023

Choose a reason for hiding this comment

pandaapo Dec 5, 2023 • edited Loading

Choose a reason for hiding this comment

wizardzhang Dec 5, 2023

Choose a reason for hiding this comment

pandaapo left a comment

Choose a reason for hiding this comment

wizardzhang commented Dec 5, 2023

pandaapo commented Dec 5, 2023 • edited Loading

pandaapo Dec 5, 2023

Choose a reason for hiding this comment

wizardzhang Dec 6, 2023 • edited Loading

Choose a reason for hiding this comment

pandaapo left a comment

Choose a reason for hiding this comment

mxsm left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 6, 2023 • edited Loading

Codecov Report

wizardzhang commented Dec 6, 2023

Pil0tXia left a comment

Choose a reason for hiding this comment

wizardzhang commented Dec 8, 2023

pandaapo Dec 5, 2023 •

edited

Loading

pandaapo commented Dec 5, 2023 •

edited

Loading

wizardzhang Dec 6, 2023 •

edited

Loading

codecov bot commented Dec 6, 2023 •

edited

Loading