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

Improve testability of Main.java while fixing issue #314 #316

Merged
merged 2 commits into from Jan 4, 2017

Conversation

Projects
None yet
3 participants
@jmcgarr
Contributor

jmcgarr commented Nov 6, 2016

My goal for this pull request is to allow the jbake/jbake-gradle-plugin an API to start the JettyServer. While adding this functionality to the jbake-gradle-plugin, I observed a bug (#314) that prevents the JettyServer to start up with a custom output directory as well as providing the watcher with a custom source directory. The fix for this issue was fairly straightforward.

While adding this change to the Main.run() method, I recognized an opportunity to refactor the code to make it more testable. This allowed me to write tests for the failure cases I detected and provide fixes for them.

jmcgarr added some commits Nov 4, 2016

Fix Issue #314 - JettyServer fails to serve cmdline destination dir
The Main.run() method is responsible for configuring the destination directory for JBake's built in JettyServer. This commit is to ensure that it considers the desitination directory passed in from the commandline. Still need to add tests.
Refactoring Main.java to improve testability
Took a stab at refactoring some of the functionality in Main.java to create seams for testing. This requires removing some static methods, converting private methods to classes that can be mocked, overloading methods, creating constructors to externalize the new dependencies.
@coveralls

This comment has been minimized.

Show comment
Hide comment
@coveralls

coveralls Nov 6, 2016

Coverage Status

Coverage increased (+1.7%) to 72.08% when pulling f4e198b on jmcgarr:fix/issue-314 into 6dcd780 on jbake-org:master.

coveralls commented Nov 6, 2016

Coverage Status

Coverage increased (+1.7%) to 72.08% when pulling f4e198b on jmcgarr:fix/issue-314 into 6dcd780 on jbake-org:master.

@jonbullock jonbullock added this to the v2.5.1 milestone Nov 13, 2016

@jonbullock jonbullock self-assigned this Nov 13, 2016

@jonbullock

This comment has been minimized.

Show comment
Hide comment
@jonbullock

jonbullock Nov 13, 2016

Member

Just been reviewing this change and I'm assuming this breaks the execution of the -s option when it's used on it's own with just a single path parameter?

Member

jonbullock commented Nov 13, 2016

Just been reviewing this change and I'm assuming this breaks the execution of the -s option when it's used on it's own with just a single path parameter?

@jmcgarr

This comment has been minimized.

Show comment
Hide comment
@jmcgarr

jmcgarr Nov 18, 2016

Contributor

It shouldn't break this and there is a test case to make sure that option still works.

Contributor

jmcgarr commented Nov 18, 2016

It shouldn't break this and there is a test case to make sure that option still works.

String[] args = {"-s", "build/jbake"};
main.run(args);
verify(mockJetty).run("output","8820");

This comment has been minimized.

@jonbullock

jonbullock Jan 3, 2017

Member

This is the scenario I was referring to in my previous comment, in v2.5.0 the result of this execution jbake -s build/jbake would be that the contents of build/jbake would be served out not output. The underlying issue is the sharing of the source & destination values for different cmd line options.

@jonbullock

jonbullock Jan 3, 2017

Member

This is the scenario I was referring to in my previous comment, in v2.5.0 the result of this execution jbake -s build/jbake would be that the contents of build/jbake would be served out not output. The underlying issue is the sharing of the source & destination values for different cmd line options.

@jonbullock

This comment has been minimized.

Show comment
Hide comment
@jonbullock

jonbullock Jan 3, 2017

Member

I've added a line comment for the scenario I was referring to and how 2.5.0 currently behaves. While I don't want to break existing functionality or at least the way JBake behaved, I do want to merge this in as it improves the testability of the launch commands. I'll do a bit more research and see if I can come up with a quick fix until a better overhaul of the command line interface can be implemented.

Member

jonbullock commented Jan 3, 2017

I've added a line comment for the scenario I was referring to and how 2.5.0 currently behaves. While I don't want to break existing functionality or at least the way JBake behaved, I do want to merge this in as it improves the testability of the launch commands. I'll do a bit more research and see if I can come up with a quick fix until a better overhaul of the command line interface can be implemented.

@jonbullock jonbullock merged commit 92fb557 into jbake-org:master Jan 4, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

jonbullock added a commit that referenced this pull request Jan 4, 2017

Change to #316
Maintained current behaviour when using server option with single path
value as well as when chaining bake and server, Jetty should use
destination path if it's been specified.
@jonbullock

This comment has been minimized.

Show comment
Hide comment
@jonbullock

jonbullock Jan 4, 2017

Member

I've added a workaround to maintain the existing behaviour while still allowing the Jetty server to serve out the destination path when combined with the bake option and explicit source & destination locations.

Thanks @jmcgarr for submitting this change 👍

Member

jonbullock commented Jan 4, 2017

I've added a workaround to maintain the existing behaviour while still allowing the Jetty server to serve out the destination path when combined with the bake option and explicit source & destination locations.

Thanks @jmcgarr for submitting this change 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment