Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CXF-9023] Increase unit test coverage on org.apache.cxf.bus #1901

Merged
merged 2 commits into from
Jun 19, 2024

Conversation

jgoodyear
Copy link
Contributor

No description provided.


import static org.junit.Assert.assertEquals;

public class ManagedBusTest {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jgoodyear the ManagedBus is covered by ManagedBusTest (systests/uncategorized)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feels odd to have classes tested outside of the package, even to have some basic smoke testing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CXF is quite old and some decisions as well, but we should not add tests for a sake of tests or/and coverage, it adds significant maintenance costs for no benefits (surely, in context of CXF).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right on.

Shall CXF-9023 & CXF-9024 be closed as 'will not fix'?

Copy link
Member

@reta reta May 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't mind, let's spend some time and find the gaps, fe. I have not found any tests for ClientLifeCycleManagerImpl but you added one (thank again for it), there could be others

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Take any portion of the PR you see value in retaining :)

@reta
Copy link
Member

reta commented May 30, 2024

@jgoodyear thanks for working on tests, I believe we should spend a bit more time on crafting more or less realistic test cases, may be under systests/* even if needed. The ones here clearly use too much mocking (sometimes it is useful) but really shouldn't, plus there is quite large overlap with systests/* that I think we should take into account. Hope it makes sense, thank you.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

Copy link
Member

@reta reta Jun 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

@reta reta merged commit 59bc6c2 into apache:main Jun 19, 2024
4 checks passed
reta pushed a commit that referenced this pull request Jun 20, 2024
* [CXF-9023] Increase unit test coverage on org.apache.cxf.bus

* Remove the tests for classes that already have enough coverage

---------

Co-authored-by: Andriy Redko <drreta@gmail.com>
(cherry picked from commit 59bc6c2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants