Print summary at the end of the run #536

Merged
merged 7 commits into from Jul 16, 2013

Conversation

Projects
None yet
3 participants
Contributor

brasmusson commented Jun 5, 2013

This PR fixes #195

Print the same kind of summary at the end of the run as Cucumber-Ruby (sub counts are only printed in >0):

x Scenarios (x1 failed, x2 skipped, x3 pending, x4 undefined, x5 passed)
y Steps (y1 failed, y2 skipped, y3 pending, y4 undefined, y5 passed)
XmY.YYYs

The new class SummaryCounter is used by the Runtime class to collect the
data for the summary. The SummaryPrinter then let the SummaryCounter
print the summary before the errors and the snippets. The printout from
the SummaryPrinter is moved to after the printout from the formatters to
make the summary printed after the progress or pretty printout.

brasmusson added some commits Jun 1, 2013

@brasmusson brasmusson Print summary at the end of the run
Print the same kind of summary at the end of the run as Cucumber-Ruby:
x Scenarios (x1 failed, x2 skipped, x3 pending, x4 undefined, x5 passed)
y Steps (y1 failed, y2 skipped, y3 pending, y4 undefined, y5 passed)
XmY.YYYs

The new class SummaryCounter is used by the Runtime class to collect the
data for the summary. The SummaryPrinter then let the SummaryCounter
print the summary before the errors and the snippets. The printout from
the SummaryPrinter is moved to after the printout from the formatters to
make the summary printed after the progress or pretty printout.
5fb1b76
@brasmusson brasmusson Use only jdk1.6 compliant constructs in the tests 3b0f7bd

sebrose was assigned Jun 10, 2013

Owner

aslakhellesoy commented Jun 10, 2013

Awesome thanks!

I have a few questions/comments that I'll add to the various lines.

@aslakhellesoy aslakhellesoy and 1 other commented on an outdated diff Jun 10, 2013

core/src/main/java/cucumber/runtime/Runtime.java
@@ -62,19 +63,29 @@ public Runtime(ResourceLoader resourceLoader, ClassLoader classLoader, RuntimeOp
}
public Runtime(ResourceLoader resourceLoader, ClassLoader classLoader, Collection<? extends Backend> backends, RuntimeOptions runtimeOptions) {
+ this(resourceLoader, classLoader, backends, runtimeOptions, new UndefinedStepsTracker());
@aslakhellesoy

aslakhellesoy Jun 10, 2013

Owner

This class isn't in a public API, so there is no need to keep old constructors for backwards compatibility. Can you cut this back to two (or preferrably - one) constructors please?

@brasmusson

brasmusson Jun 11, 2013

Contributor

It is not so easy. It is not possible to first create a UndefinedStepsTracker and the pass it to the creation of the RuntimeGlue in the same constructor, which calls another constructor, so I think both the two new constructors are needed, but they does not need to be public, the third can be private and the forth package private.
The first constructor is used by cli.Main and the second one is used by tests to pass backends to the Runtime. It is used in tests both in the runtime package and in the runtime.formatter package so need to be public.
I can cut it back to three constructors, but this will affect a hand full of tests, see the gist: https://gist.github.com/brasmusson/5755281

@aslakhellesoy aslakhellesoy and 1 other commented on an outdated diff Jun 10, 2013

core/src/main/java/cucumber/runtime/Runtime.java
this.resourceLoader = resourceLoader;
this.classLoader = classLoader;
- if (backends.isEmpty()) {
@aslakhellesoy

aslakhellesoy Jun 10, 2013

Owner

Why did you remove this?

@brasmusson

brasmusson Jun 11, 2013

Contributor

That was just a mistake. The assignments to fields ware performed all over the constructor, before the check for empty backends, after this check, and after loading backends, so when I moved the code to the new constructor I missed this. Which on the other hand points out that there is a test missing, since it could be removed without any test failure :-).
I'll put it back and add the test.

@aslakhellesoy aslakhellesoy and 1 other commented on an outdated diff Jun 10, 2013

...in/java/cucumber/runtime/snippets/SummaryPrinter.java
printErrors(runtime);
printSnippets(runtime);
}
+ private void printSummary(cucumber.runtime.Runtime runtime) {
+ runtime.getSummaryCounter().printSummary(out);
@aslakhellesoy

aslakhellesoy Jun 10, 2013

Owner

Tell don't ask. Remove getSummaryCounter() and add a Runtime.printSummary(out) method instead.

@brasmusson

brasmusson Jun 11, 2013

Contributor

I'll fix it.

@aslakhellesoy aslakhellesoy and 1 other commented on an outdated diff Jun 10, 2013

core/src/main/java/cucumber/runtime/Runtime.java
@@ -292,4 +307,18 @@ public static boolean isPending(Throwable t) {
public void writeStepdefsJson() throws IOException {
glue.writeStepdefsJson(runtimeOptions.featurePaths, runtimeOptions.dotCucumber);
}
+
+ public SummaryCounter getSummaryCounter() {
@aslakhellesoy

aslakhellesoy Jun 10, 2013

Owner

Remove this (see below)

@brasmusson

brasmusson Jun 11, 2013

Contributor

I'll fix it

@brasmusson brasmusson Tell don't ask when printing summary at the end of the run
Let the SummaryPrinter tell the Runtime to print the summary instead of
asking for the SummaryCounter.

Put back the check for an emtpy list of backends in the Runtime ctor,
which was removed by mistake. Add test to check that a CucumberException
is thrown when the list of backends is emtpy.

Use Long for duration arguments in SummaryCounter, to conform to the
return type of Result.getDuration().
07902e1
Contributor

brasmusson commented Jun 11, 2013

It is possible to cut the number of constructors in Runtime to one constructor and one factory method, but is it the right way to go? See https://gist.github.com/brasmusson/5755281

I would like encouragement that that change actually is an improvement, before I push up that change.

Owner

sebrose commented Jun 23, 2013

@brasmusson - have you seen my comment/query on your gist? https://gist.github.com/brasmusson/5755281

Contributor

brasmusson commented Jun 24, 2013

@sebrose - I'm sorry I have missed it. I'm still learning about what notifications github will send mail for, and what I need to look out for manually. In short, since the same UndefinedStepTracker instance is shared between the Runtime and the RuntimeGlue, I did not see another options to allow for a RuntimeGlue to be injected, than to create both the UndefinedStepTracker and the RuntimeGlue in the factory method and send them both to the Runtime constructor.

Since the gist in all looked fine to you, I will add a commit to this pull request with the change to only have one constructor in the Runtime and to have a factory method as the normal way to construct a Runtime (when not injecting an alternative/mocked RuntimeGlue)

@brasmusson brasmusson Minimize the number of constructors in the Runtime class
To minimize the number of constructors in the Runtime class, and to
handle that by default a UndefinedStepTracker is created and then
injected both into the Runtime and the RuntimeGlue, create a factory
method as the main entry. Expose additional conveniece factory methods
for testing in the core module in RuntimeTest.
c8feb10
Contributor

brasmusson commented Jun 24, 2013

Now I got a build failure on openjdk6 for commit c8feb10, and the logs are truncated (both for the failed build and the passed build, so I cannot see why the build failed, but the log says that the Spring module started to build, so the changed modules Core, Java and Junit had been built and passed), and I cannot restart the build, so now I do not know what to do.
Since commit c8feb10 basically is the commit b0e45a7 of PR #544 cherry-picked onto this branch, and the Travis build for commit b0e45a7 passed, there is some reason to believe that it was a Travis glitch that caused the failed build.

Contributor

brasmusson commented Jun 24, 2013

After finally succeeded to get a complete log from Travis, I see that it was the deployment of snapshots to maven central that failed:

travis_fold:end:install
$ mvn -q clean deploy --settings .travis-settings.xml -Dno.gem.deploy=true
Jun 24, 2013 9:52:22 AM org.apache.commons.httpclient.auth.AuthChallengeProcessor selectAuthScheme
INFO: basic authentication scheme selected
Jun 24, 2013 9:52:23 AM org.apache.commons.httpclient.HttpMethodDirector processWWWAuthChallenge
INFO: Failure authenticating with BASIC 'Sonatype Nexus Repository Manager'@oss.sonatype.org:443
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-deploy-plugin:2.7:deploy (default-deploy) on project cucumber-jvm: Failed to deploy artifacts: Could not transfer artifact info.cukes:cucumber-jvm:pom:1.1.4-20130624.095222-2 from/to sonatype-nexus-snapshots (https://oss.sonatype.org/content/repositories/snapshots/): Failed to transfer file: https://oss.sonatype.org/content/repositories/snapshots//info/cukes/cucumber-jvm/1.1.4-SNAPSHOT/cucumber-jvm-1.1.4-20130624.095222-2.pom. Return code is: 401 -> [Help 1]

On the other hand a build on a pull request should not be deployed, so it might be just as well that the deployment failed. Somehow the Travis set up for Cucumber-JVM ought to be change to exclude deployment of builds for pull requests.
Anyway, commit c8feb10 should be considered as "Good to merge" as far as the Travis build is concerned.

@brasmusson brasmusson Localise the UndefinedStepTracker in the Runtime
Localise the UndefinedStepTracker in the Runtime and remove that
argument from the constructor. Use that the optionalGlue argument is
null, as the cue to let the Runtime create the default RuntimeGlue.
When the optionalGlue argument is not null, the glue injected with that
argument will be used by the Runtime.
3a77cff
Contributor

brasmusson commented Jun 25, 2013

@sebrose I looked over the Runtime constructor again. In 3a77cff the UndefinedStepTracker is localized within the Runtime. I am not totally happy about using when the optionalGlue argument is null, as the cue for the Runtime constructor to create the default RuntimeGlue. On the other hand, normally the Runtime is constructed through factory method, so the constructor is only intended to be used when backends and/or glue needs to be injected for testing purposes.

RuntimeOptions.monochrome is now private, since checkin 65f3bd6

Owner

aslakhellesoy replied Jun 27, 2013

Just add an isMonochrome() method if you need it exposed.

Contributor

brasmusson replied Jun 28, 2013

Fixed in new commit on the PR.

@brasmusson brasmusson Add RuntimeOptions.isMonochrome(), rm Runtime factory methods
Add RuntimeOptions.isMonochorme(), since commit 65f3bd6 on the master
branch change RuntimeOptions to have private fields and getters.

Basically revert commit c8feb10. Since the new android support on the
master banch sends backends to the Runtime constructor, which
previously only was done in tests, the change to use factory methods in
Runtime no longer make sence.
442c4ed
Contributor

brasmusson commented Jun 29, 2013

When I merged cucumber-jvm:master into this PR branch and resolved that RuntimeOptions is change to use private fields and getter, I discovered that my solution to minimize the number of constructors in Runtime does not work with the new android module. The android module injects backends into the Runtime, which previously only been done in tests (see https://travis-ci.org/cucumber/cucumber-jvm/builds/8535311).
As a result I more or less reverted c8feb10 with 442c4ed. The Runtime now has three constructors:

  • One used by core/src/main/java/cucumber/api/cli/Main.java and junit/src/main/java/cucumber/api/junit
    /Cucumber.java
  • One used by cucumber-jvm/android/src/cucumber/api/android/CucumberInstrumentation.java and many tests to inject backends into the Runtime
  • One used by other tests to inject a glue (and backends) into the Runtime.

It is possible to only have one constructor in the Runtime, but then the production classes that instantiates the Runtime would need to pass null for the backends and glue parameter to say "load backends and instantiate a RuntimeGlue as usual". I think that would be confusing, passing null is more like saying "there should be no backends nor glue in the Runtime".

Commit 442c4ed was not build by Travis, maybe because git get confused when both cucumber-jvm:master and this PR branch add methods (getters) last in the RuntimeOptions and report merge a conflict, but after manually merging cucumber-jvm:master into the PR branch Travis builds it successfully.

Contributor

brasmusson commented Jul 8, 2013

@sebrose Please tell me if there is more a I need to do in this pull request, because I don't see that there is.

Contributor

brasmusson commented Jul 16, 2013

@aslakhellesoy Just give me a go ahead, and I merge this PR.

Owner

aslakhellesoy commented Jul 16, 2013

LGTM!

@brasmusson brasmusson added a commit that referenced this pull request Jul 16, 2013

@brasmusson brasmusson Merge pull request #536 from brasmusson/summary-printout
Merge pull request #536 Print summary at the end of the run.
This fixes #195. Update the History.md.
f63f6c3

@brasmusson brasmusson merged commit 40a223a into cucumber:master Jul 16, 2013

1 check passed

default The Travis CI build passed
Details

brasmusson deleted the brasmusson:summary-printout branch Aug 3, 2013

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