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

Fixes #2510: Remove ExpectedException from internal test suite #2518

Merged
merged 72 commits into from Dec 24, 2021

Conversation

temp-droid
Copy link
Contributor

@temp-droid temp-droid commented Dec 16, 2021

Checklist

  • Read the contributing guide
  • PR should be motivated, i.e. what does it fix, why, and if relevant how
  • If possible / relevant include an example in the description, that could help all readers
    including project members to get a better picture of the change
  • Avoid other runtime dependencies
  • Meaningful commit history ; intention is important please rebase your commit history so that each
    commit is meaningful and help the people that will explore a change in 2 years
  • The pull request follows coding style
  • Mention Fixes #<issue number> in the description if relevant
  • At least one commit should mention Fixes #<issue number> if relevant

@codecov-commenter
Copy link

codecov-commenter commented Dec 16, 2021

Codecov Report

Merging #2518 (dc2231c) into main (95bc5b2) will not change coverage.
The diff coverage is n/a.

❗ Current head dc2231c differs from pull request most recent head 367fb12. Consider uploading reports for the commit 367fb12 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##               main    #2518   +/-   ##
=========================================
  Coverage     86.66%   86.66%           
  Complexity     2781     2781           
=========================================
  Files           320      320           
  Lines          8344     8344           
  Branches       1022     1022           
=========================================
  Hits           7231     7231           
  Misses          842      842           
  Partials        271      271           
Impacted Files Coverage Δ
src/main/java/org/mockito/Mockito.java 92.20% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95bc5b2...367fb12. Read the comment docs.

@temp-droid
Copy link
Contributor Author

Hey @TimvdLippe!

Outside one ExpectedException case in SpyAnnotationTest that I'm unsure of (not used in the past?) and one @Test(expected… in the osgi-test I'm not confortable touching, the rest should be replaced.

Before continuing replacing the try fail finally cases, I wanted to check with you if it wouldn't be best to create another PR for those. We are already at 72 commits for this one and I counted more than 380 occurrences in ~120 files for fail().

I feel like this would be too much to review in a single PR, but you tell me.

@TimvdLippe
Copy link
Contributor

Yes. This PR is already massive, so let's cut it here. I hope to have time on Friday to review. Thanks for your effort!

Note that in this case I am okay with so many commits, since it is mechanical. However, for other PRs let's try to keep them small 😄

@temp-droid temp-droid changed the title Fixes #2510: Remove ExpectedException from internal test suite (WIP) Fixes #2510: Remove ExpectedException from internal test suite Dec 22, 2021
@temp-droid temp-droid marked this pull request as ready for review December 22, 2021 20:08
@temp-droid
Copy link
Contributor Author

Do you have suggestions how to approach the new PR? First creating an issue to link it to? Or referencing the same one?

I'm afraid it might create confusion to have 2 PRs for the same issue qua closing.

@TimvdLippe
Copy link
Contributor

Let's file a new issue for that. Thanks!

Copy link
Contributor

@TimvdLippe TimvdLippe left a comment

Choose a reason for hiding this comment

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

Thanks for the cleanup!

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.

None yet

3 participants