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.

Sign up for GitHub

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

Jump to bottom

Rename armeria-testing-junit module to armeria-junit5 #2829

Merged
merged 3 commits into from Jun 24, 2020
Merged

Rename armeria-testing-junit module to armeria-junit5 #2829

merged 3 commits into from Jun 24, 2020

Conversation

minwoox
Copy link
Member

@minwoox minwoox commented Jun 23, 2020 •
edited

Motivations:
We have decided to change module names. See #2677 (comment)

Modifications:

  • Rename armeria-testing-junit to armeria-junit5
  • Rename armeria-testing-junit4 to armeria-junit4
  • Move all 3 classes in armeria-testing-common to armeria-junit5 and armeria-junit4.

Result:

  • Close Refine module structure #2677
  • You now have to use armeria-junit5 instead of armeria-testing-junit
  • You now have to use armeria-junit4 instead of armeria-testing-junit4
  • armeria-testing-common is gone.

@minwoox
Rename armeria-testing-junit module to armeria-junit5
cbdc716 
Motivations:
We have decided to change module names. See line#2677 (comment)

Modifications:
- Rename `armeria-testing-junit` to `armeria-junit5`
- Rename `armeria-testing-junit4` to `armeria-junit4`
- Move all 3 classes in `armeria-testing-common` to core module.
  - It's a bit weird to have testing classes in the core module.
    However, they're only 3 classes so it should be fine. If they grow, we will revisit this later.

Result:
- Close line#2677
- You now have to use `armeria-junit5` instead of `armeria-testing-junit`
- You now have to use `armeria-junit4` instead of `armeria-testing-junit4`
- `armeria-testing-common` is gone.
@minwoox minwoox added this to the 0.99.8 milestone Jun 23, 2020
@minwoox minwoox mentioned this pull request Jun 23, 2020
Closed
@anuraaga
Copy link
Collaborator

Why not keep armeria-testing-common? Or alternatively just copy in the source into each of junit4 and junit5? IMO it's more than weird that core has testing classes, it breaks an important principal of code structure where testing classes shouldn't be mixed with business logic ever.

@codecov
Copy link

codecov bot commented Jun 23, 2020 •
edited

Codecov Report

Merging #2829 into master will increase coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #2829   +/-   ##
=========================================
  Coverage     72.73%   72.74%           
+ Complexity    12060    12044   -16     
=========================================
  Files          1069     1069           
  Lines         46897    46848   -49     
  Branches       5873     5866    -7     
=========================================
- Hits          34111    34078   -33     
+ Misses         9775     9766    -9     
+ Partials       3011     3004    -7
Impacted Files Coverage Δ Complexity Δ
...ting/junit4/common/AbstractEventLoopGroupRule.java 100.00% <ø> (ø) 4.00 <0.00> (?)
...eria/testing/junit4/common/EventLoopGroupRule.java 0.00% <ø> (ø) 0.00 <0.00> (?)
...p/armeria/testing/junit4/common/EventLoopRule.java 100.00% <ø> (ø) 5.00 <0.00> (?)
...sting/junit4/server/SelfSignedCertificateRule.java 46.15% <ø> (ø) 6.00 <0.00> (?)
...corp/armeria/testing/junit4/server/ServerRule.java 64.51% <ø> (ø) 13.00 <0.00> (?)
...a/internal/testing/EventLoopGroupRuleDelegate.java 81.25% <ø> (ø) 4.00 <0.00> (?)
...nal/testing/SelfSignedCertificateRuleDelegate.java 42.02% <ø> (ø) 10.00 <0.00> (?)
...p/armeria/internal/testing/ServerRuleDelegate.java 78.57% <ø> (ø) 32.00 <0.00> (?)
...sting/junit/common/AbstractAllOrEachExtension.java 100.00% <ø> (ø) 10.00 <0.00> (?)
.../junit/common/AbstractEventLoopGroupExtension.java 81.81% <ø> (ø) 4.00 <0.00> (?)
... and 25 more

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 28bc1e6...5fd10fa. Read the comment docs.

@minwoox
Copy link
Member Author

minwoox commented Jun 23, 2020

Or alternatively just copy in the source into each of junit4 and junit5?

Yeah, let me put them in junit5 and copy into gen-src using Gradle. 😄

@minwoox
Copy link
Member Author

minwoox commented Jun 23, 2020

@anuraaga Fixed. PTAL. 😉

ikhoon
ikhoon approved these changes Jun 23, 2020
View reviewed changes
Copy link
Contributor

@ikhoon ikhoon left a comment

Choose a reason for hiding this comment

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

Thanks! @minwoox

trustin
trustin approved these changes Jun 23, 2020
View reviewed changes
Copy link
Member

@trustin trustin left a comment

Choose a reason for hiding this comment

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

Thanks a lot, @minwoox !

@trustin
Copy link
Member

trustin commented Jun 23, 2020

@minwoox Could you update the PR description?

@trustin
Copy link
Member

trustin commented Jun 23, 2020

Ah, could you rename copyFiles to generateSources so that ./gradlew generateSources generates everything to make IDE happy?

@minwoox
Copy link
Member Author

minwoox commented Jun 24, 2020

@trustin Done. 😄

@trustin trustin merged commit 618875b into line:master Jun 24, 2020
@minwoox minwoox deleted the renamed_junit branch June 24, 2020 08:47
fmguerreiro pushed a commit to fmguerreiro/armeria that referenced this pull request Sep 19, 2020
@minwoox
Rename armeria-testing-junit module to armeria-junit5 (line#2829)
ac03d1b 
Motivations:
We have decided to change module names. See line#2677 (comment)

Modifications:
- Rename `armeria-testing-junit` to `armeria-junit5`
- Rename `armeria-testing-junit4` to `armeria-junit4`
- Move all 3 classes in `armeria-testing-common` to `armeria-junit5` and `armeria-junit4`.

Result:
- Close line#2677
- You now have to use `armeria-junit5` instead of `armeria-testing-junit`
- You now have to use `armeria-junit4` instead of `armeria-testing-junit4`
- `armeria-testing-common` is gone.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trustin trustin trustin approved these changes

@anuraaga anuraaga anuraaga approved these changes

@ikhoon ikhoon ikhoon approved these changes

Assignees
No one assigned
Labels
breaking change
Projects
None yet
Milestone
0.99.8
Development

Successfully merging this pull request may close these issues.

Refine module structure
4 participants
@minwoox @anuraaga @trustin @ikhoon