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

feat(configFactory): be able to access junit-context during config of the server #17

Merged
merged 1 commit into from
Aug 15, 2019

Conversation

davinkevin
Copy link
Contributor

@davinkevin davinkevin commented Aug 13, 2019

Closes #9

@lanwen
Copy link
Owner

lanwen commented Aug 13, 2019

yea, looks good, thx for the PR, please continue

@davinkevin
Copy link
Contributor Author

If this is ok for you, could you say to me what you would like me to add to this PR to be accepted?

@davinkevin davinkevin force-pushed the 9-create-with-extension-context branch from 253f708 to bc740c5 Compare August 14, 2019 06:05
@davinkevin
Copy link
Contributor Author

I've added one unit test about this new feature. If this is enough from your point of view, you can merge it 👍

@@ -14,9 +15,9 @@
*/
class WiremockFactory {

public WireMockServer createServer(final Wiremock mockedServer) {
public WireMockServer createServer(final Wiremock mockedServer, ExtensionContext extensionContext) {
Copy link
Owner

Choose a reason for hiding this comment

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

can I ask you to keep the previous signature as well? To not break the binary compatibility. Thus you can avoid changes on the tests, adding nulls where the context should be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, and I've extracted the instantiation of the factory inside a private method.

BTW, the method newInstance() is deprecated since Java 9:
image

Is it ok for you?

Copy link
Owner

Choose a reason for hiding this comment

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

yeah, j8 is the main target for now still, but thx for pointing

@lanwen
Copy link
Owner

lanwen commented Aug 15, 2019

please rebase on master, fixed an issue with the build

@davinkevin davinkevin force-pushed the 9-create-with-extension-context branch from bc740c5 to 42c1796 Compare August 15, 2019 10:27
@codecov
Copy link

codecov bot commented Aug 15, 2019

Codecov Report

Merging #17 into master will increase coverage by 0.27%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master      #17      +/-   ##
============================================
+ Coverage     93.75%   94.02%   +0.27%     
- Complexity       24       28       +4     
============================================
  Files             7        7              
  Lines            64       67       +3     
  Branches          2        2              
============================================
+ Hits             60       63       +3     
  Misses            1        1              
  Partials          3        3
Impacted Files Coverage Δ Complexity Δ
.../lanwen/wiremock/config/WiremockConfigFactory.java 100% <100%> (ø) 2 <2> (+2) ⬆️
...n/java/ru/lanwen/wiremock/ext/WiremockFactory.java 100% <100%> (ø) 6 <3> (+2) ⬆️
.../java/ru/lanwen/wiremock/ext/WiremockResolver.java 93.75% <100%> (ø) 7 <0> (ø) ⬇️

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 44be487...d5f8095. Read the comment docs.

@davinkevin davinkevin force-pushed the 9-create-with-extension-context branch from 42c1796 to d5f8095 Compare August 15, 2019 10:31
@davinkevin
Copy link
Contributor Author

Rebase done on master, I let you decide if all is ok for you.

I will try to work on #16 if possible 😉

@lanwen lanwen merged commit 0b8853e into lanwen:master Aug 15, 2019
@lanwen
Copy link
Owner

lanwen commented Aug 15, 2019

great, thank you, will release that shortly

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.

It would be convenient in the method WiremockConfigFactory.create() transmit ExtensionContext
2 participants