You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@dubbo.apache.org by GitBox <gi...@apache.org> on 2022/06/24 13:49:49 UTC
[GitHub] [dubbo] Codegass opened a new issue, #10214: Refactor `ChannelBufferStreamTest.testAll` to improve the test experience
Codegass opened a new issue, #10214:
URL: https://github.com/apache/dubbo/issues/10214
<!-- If you need to report a security issue please visit https://github.com/apache/dubbo/security/policy -->
- [X] I have searched the [issues](https://github.com/apache/dubbo/issues) of this repository and believe that this is not a duplicate.
## Describe the proposal
<!-- Please use this for a concrete design proposal for functionality. -->
<!-- If you just want to request a new feature and discuss the possible business value, create a Feature Request. -->
### Motivation
<!--
The case [testAll](https://github.com/apache/dubbo/blob/d41a0448c70ac11e1d3df4b80ba9fd00829097a6/dubbo-remoting/dubbo-remoting-api/src/test/java/org/apache/dubbo/remoting/buffer/ChannelBufferStreamTest.java#L30) as the only test case in the ChannelBufferStreamTest is testing the same functions `ChannelBufferOutputStream()` and `ChannelBufferInputStream()` multiple times with different scenarios (inputs) in the exact same test case.
Although the test case's logic is flawless, refactoring the `testAll` case into multiple unit tests can help to improve the readability of the test cases, make developers easy to understand the test target from the test case name. Right now the name `testAll` is too general, and putting multiple testing scenarios in one test case can make it hard for developers to locate more bugs in one CI\CD cycle.
-->
The case [testAll](https://github.com/apache/dubbo/blob/d41a0448c70ac11e1d3df4b80ba9fd00829097a6/dubbo-remoting/dubbo-remoting-api/src/test/java/org/apache/dubbo/remoting/buffer/ChannelBufferStreamTest.java#L30) as the only test case in the `ChannelBufferStreamTest` is testing the same functions `ChannelBufferOutputStream()` and `ChannelBufferInputStream()` multiple times with different scenarios (inputs) in the exact same test case.
Refactoring the [testAll](https://github.com/apache/dubbo/blob/d41a0448c70ac11e1d3df4b80ba9fd00829097a6/dubbo-remoting/dubbo-remoting-api/src/test/java/org/apache/dubbo/remoting/buffer/ChannelBufferStreamTest.java#L30) case into multiple unit tests can help to improve the readability of the test cases. Right now the name testAll is quite general. After the refactoring, each test case will be given a more meaningful name to represent the test scenario. This will make it easier to understand the test target from the test case name. In addition, putting multiple testing scenarios in one test case makes it harder for us to locate bugs in one CI\CD cycle.
### Proposed changes
I suggest refactoring this case into 5 simple unit tests with one testing scenario for each unit test case.
* `testChannelBufferOutputStreamNull`
* Extract the [line 33 to 38](https://github.com/apache/dubbo/blob/d41a0448c70ac11e1d3df4b80ba9fd00829097a6/dubbo-remoting/dubbo-remoting-api/src/test/java/org/apache/dubbo/remoting/buffer/ChannelBufferStreamTest.java#L33-L38) as the first test case to test the `ChannelBufferOutputStream()` with Null input.
* `testChannelBufferOutputStreamWithBuffer`
* Extract the [line 40 to 45](https://github.com/apache/dubbo/blob/d41a0448c70ac11e1d3df4b80ba9fd00829097a6/dubbo-remoting/dubbo-remoting-api/src/test/java/org/apache/dubbo/remoting/buffer/ChannelBufferStreamTest.java#L40-L45) as the second test case to test the functions of `ChannelBufferOutputStream()` with normal buffer input.
* `testChannelBufferInputStreamNull`
* Extract the [line 47 to line 52](https://github.com/apache/dubbo/blob/d41a0448c70ac11e1d3df4b80ba9fd00829097a6/dubbo-remoting/dubbo-remoting-api/src/test/java/org/apache/dubbo/remoting/buffer/ChannelBufferStreamTest.java#L47-L52) as the third test case to test the `ChannelBufferInputStream()` with Null input.
* `testChannelBufferInputStreamNullwithZeroCapacity`
* Extract the [line 54 to line 59](https://github.com/apache/dubbo/blob/d41a0448c70ac11e1d3df4b80ba9fd00829097a6/dubbo-remoting/dubbo-remoting-api/src/test/java/org/apache/dubbo/remoting/buffer/ChannelBufferStreamTest.java#L54-L59) as the third test case to test the `ChannelBufferInputStream()` with Null input and zero capacity.
* `testChannelBufferInputStreamWithNegativeBufferCapacity`
* Extract the [line 61 to 65](https://github.com/apache/dubbo/blob/d41a0448c70ac11e1d3df4b80ba9fd00829097a6/dubbo-remoting/dubbo-remoting-api/src/test/java/org/apache/dubbo/remoting/buffer/ChannelBufferStreamTest.java#L61-L65) as the fourth test case to test the `ChannelBufferInputStream()` with normal buffer input and negative capacity setting.
* `testChannelBufferInputStreamWithOversizeBufferCapacity`
* Extract the [line 67 to 71](https://github.com/apache/dubbo/blob/d41a0448c70ac11e1d3df4b80ba9fd00829097a6/dubbo-remoting/dubbo-remoting-api/src/test/java/org/apache/dubbo/remoting/buffer/ChannelBufferStreamTest.java#L67-L71) as the fourth test case to test the `ChannelBufferInputStream()` with normal buffer input and oversize capacity setting.
* `testChannelBufferInputStreamWithBuffer`
* Extract the [line 73 to 102](https://github.com/apache/dubbo/blob/d41a0448c70ac11e1d3df4b80ba9fd00829097a6/dubbo-remoting/dubbo-remoting-api/src/test/java/org/apache/dubbo/remoting/buffer/ChannelBufferStreamTest.java#L73-L102) as the fifth test case to test the functions of `ChannelBufferInputStream()` with normal buffer input.
Additionally, `ChannelBuffer buf = ChannelBuffers.dynamicBuffer()` at [line 31](https://github.com/apache/dubbo/blob/d41a0448c70ac11e1d3df4b80ba9fd00829097a6/dubbo-remoting/dubbo-remoting-api/src/test/java/org/apache/dubbo/remoting/buffer/ChannelBufferStreamTest.java#L31) can be extracted as the field variable and put in the `@Before setup()` function. And it can also be considered to be mocked by a Mocking framework(Mokito), since the `ChannelBuffers.dynamicBuffer` is using the basic buffer's write and read function.
For each of the test cases containing try-catch structure, I suggest replacing the try-catch with `@Test(expected = Exception.class)` annotation, where the Exception can be replaced as the specific exception for each case. Here is an example for the first case `testChannelBufferInputStreamNull`.
Before
```java
try {
new ChannelBufferOutputStream(null);
fail();
} catch (NullPointerException e) {
// Expected
}
```
After
```java
@Test(expected = NullPointerException.class)
public void testChannelBufferInputStreamNull() throws Exception {
new ChannelBufferOutputStream(null);
}
```
The same format can also be applied for `testChannelBufferInputStreamNull`, `testChannelBufferInputStreamNullwithZeroCapacity`, `testChannelBufferInputStreamWithNegativeBufferCapacity`, `testChannelBufferInputStreamWithOversizeBufferCapacity`.
After this refactoring, all the test scenarios will belong to one unit test case and can be evaluated in one cycle of testing.
This will also improve the readability of the test cases, make developers easy to understand the test target from the test case name.
Hope this proposal can be helpful, and if possible, I would be more than happy to try the refactoring. (If not, comments are still appreciated.)
--
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@dubbo.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] cheese8 commented on issue #10214: Refactor `ChannelBufferStreamTest.testAll` to improve the test experience
Posted by GitBox <gi...@apache.org>.
cheese8 commented on issue #10214:
URL: https://github.com/apache/dubbo/issues/10214#issuecomment-1171013466
It's a very good refactor idea, Sorry for I tried this.
--
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@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] AlbumenJ commented on issue #10214: Refactor `ChannelBufferStreamTest.testAll` to improve the test experience
Posted by GitBox <gi...@apache.org>.
AlbumenJ commented on issue #10214:
URL: https://github.com/apache/dubbo/issues/10214#issuecomment-1166790399
Nice, please go ahead.
--
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@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] wangchengming666 closed issue #10214: Refactor `ChannelBufferStreamTest.testAll` to improve the test experience
Posted by GitBox <gi...@apache.org>.
wangchengming666 closed issue #10214: Refactor `ChannelBufferStreamTest.testAll` to improve the test experience
URL: https://github.com/apache/dubbo/issues/10214
--
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@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org
[GitHub] [dubbo] wangchengming666 commented on issue #10214: Refactor `ChannelBufferStreamTest.testAll` to improve the test experience
Posted by GitBox <gi...@apache.org>.
wangchengming666 commented on issue #10214:
URL: https://github.com/apache/dubbo/issues/10214#issuecomment-1175141447
solved by #10244, so close.
--
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@dubbo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@dubbo.apache.org
For additional commands, e-mail: notifications-help@dubbo.apache.org