mbien commented on pull request #75:
URL: https://github.com/apache/roller/pull/75#issuecomment-846322119
> > i don't feel that it adds enough benefits in this particular case, to
justify a new test dependency (just for one small junit test).
>
> For this one at least it adds some benefit I think for example:
>
> `assertThat(readIpBanList()).containsOnly("10.0.0.1");`
>
> The standard assertion APIs don't provide such a collection verification
if I'm not mistaken.
```
assertTrue(list.contains("10.0.0.1"));
assertTrue(list.size() == 1);
```
or if you prefer one line
```
assertEquals(List.of("10.0.0.1"), list);
```
> Also to me having a new test dependency is not such a big deal but is
there something concerning about it?
Its far less problematic than a runtime dependency - i agree. Still, it has
to pull its weight. Old projects tend to accumulate dependencies and removing
them is always a little bit more difficult than adding new ones. More
dependencies mean more maintenance time spent to keep them updated and detect
abandoned projects to migrate away from.
The red flags here are that this library is only used by a single (small)
test, and it is also not convincing to me that it is really necessary for that
test.
However the impact is little (and it has no transitive dependencies), so if
you really want it, I am ok to merge your changes. (thanks again for the
contribution btw, esp also for adding a test case)
--
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
