You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by "strangelookingnerd (via GitHub)" <gi...@apache.org> on 2024/07/02 16:58:28 UTC
[PR] Cleanup test classes [httpcomponents-core]
strangelookingnerd opened a new pull request, #470:
URL: https://github.com/apache/httpcomponents-core/pull/470
This PR cleans up the test classes. The proposed changes include:
**Remove superfluous exceptions in throws clauses**
Superfluous exceptions within throws clauses have negative effects on the readability and maintainability of the code.
**Test classes and methods should have default package visibility**
It is ecommended to use the default package visibility to improve readability:
>
> Test classes, test methods, and lifecycle methods are not required to be public, but they must not be private.
It is generally recommended to omit the public modifier for test classes, test methods, and lifecycle methods unless there is a technical reason for doing so – for example, when a test class is extended by a test class in another package. Another technical reason for making classes and methods public is to simplify testing on the module path when using the Java Module System.
> — JUnit5 User Guide
**Fix assertion argument orders**
To me it seems like some assertions where not correctly upgraded when JUnit5 was introduced, where the order of arguments for some `Assertions` methods was changed.
**Remove duplicate tests**
Some test methods were implemented twice (=identical) under different names in the same class.
**Make sure tests have at least one assertion**
A test case should at least have one assertion made.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Cleanup test classes [httpcomponents-core]
Posted by "strangelookingnerd (via GitHub)" <gi...@apache.org>.
strangelookingnerd commented on PR #470:
URL: https://github.com/apache/httpcomponents-core/pull/470#issuecomment-2191634860
> > Make sure tests have at least one assertion
>
> I am aware of this recommendation but I always felt it was not justified / too extreme.
I myself am on the edge for this. I do agree that there may be some exceptions, where the goal of a test is given by its name and it does not ultimately need an assertion. However when looking at tests like these:
```java
@Test
void testConstructor() {
new MyObject();
}
```
I personally feel that this can be improved by something like this in order to make it clear to the reader what is intended to be tested:
```java
@Test
void testConstructor() {
Assertions.assertDoesNotThrow(() -> new MyObject());
}
```
This is a personal preference, however.
>
> Is there a chance you could split this PR into multiple commits? Can we start with ` Test classes and methods should have default package visibility`, `Remove duplicate tests` and `Fix assertion argument orders`?
I can defintitly try it, I will force-push the changes once I'm done and ping you 👍🏼
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Cleanup test classes [httpcomponents-core]
Posted by "strangelookingnerd (via GitHub)" <gi...@apache.org>.
strangelookingnerd commented on PR #470:
URL: https://github.com/apache/httpcomponents-core/pull/470#issuecomment-2191642879
> I would not change public vs package visibility, but that's just me, and the convention we've been using.
The thing is that right now there is a mixture (at least for the methods) of `public` and `package` that would get resolved. As for the classes, I think it helps a lot in terms of readability and would make it consistent with the recommendation of the JUnit Team.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Cleanup test classes [httpcomponents-core]
Posted by "ok2c (via GitHub)" <gi...@apache.org>.
ok2c commented on PR #470:
URL: https://github.com/apache/httpcomponents-core/pull/470#issuecomment-2191594071
Oh, that is a big one.
> Make sure tests have at least one assertion
I am aware of this recommendation but I always felt it was not justified / too extreme.
Is there a chance you could split this PR into multiple commits? Can we start with ` Test classes and methods should have default package visibility`, `Remove duplicate tests` and `Fix assertion argument orders`?
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org
Re: [PR] Cleanup test classes [httpcomponents-core]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #470:
URL: https://github.com/apache/httpcomponents-core/pull/470#issuecomment-2191613075
I would not change public vs package visibility, but that's just me, and the convention we've been using.
--
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: dev-unsubscribe@hc.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@hc.apache.org
For additional commands, e-mail: dev-help@hc.apache.org