Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Allow TestNG Cucumber runner to use composition instead of inheritance #622

Merged
merged 3 commits into from Nov 12, 2013

Conversation

Projects
None yet
3 participants
Contributor

martykube commented Nov 5, 2013

Right now to use the TestNG runner you need to inherit from AbstractTestNGCucumberTests. I'm working on a large existing project where we have a base test class that does a lot of framework setup. The only solution I had was to copy AbstractTestNGCucumberTests into my project and modify it to inherit from our base class.

This patch removes the need to inherit from AbstractTestNGCucumberTests. You can still do so if you want.

@ghost ghost assigned ffbit Nov 5, 2013

Member

ffbit commented Nov 5, 2013

@martykube great job!
That's TestNG day has come!
I've run your code code and had a look at the test report of the java-calculator-testng test project.
And I found your idea and code brilliant.
I'll merge it in a couple of days, if there no comments/questions/suggestions on your code.

off topic
I can see that TestNG becomes more and more popular.
Could you, please, provide a couple of good links or books to read on TestNG?
Thanks in advance.

Have a good day.

Contributor

martykube commented Nov 6, 2013

@ffbit Thanks for the positive feedback! I'll keep an eye out for comments/questions.suggestions.

I am sorry to say I don't have any recommendations yet on TestNG reading. This is my first encounter with TestNG... I was asked to add Cucumber to an existing Selenium framework. The framework uses TestNG so here I am :-)

I do want to add a some code examples, maybe at examples/java-calculator-testng/README.md. That might make it easier for folks to pick up Cucumber + TestNG.

@ffbit ffbit commented on the diff Nov 8, 2013

.../cucumber/api/testng/AbstractTestNGCucumberTests.java
@@ -17,28 +17,10 @@
import java.io.IOException;
@ffbit

ffbit Nov 8, 2013

Member

There are some unused imports left. Could you please remove them?

@martykube

martykube Nov 9, 2013

Contributor

will do

@ffbit ffbit and 1 other commented on an outdated diff Nov 8, 2013

...in/java/cucumber/api/testng/TestNGCucumberRunner.java
+import java.util.List;
+
+/**
+ * Glue code for running Cucumber via TestNG.
+ */
+public class TestNGCucumberRunner {
+
+ private final cucumber.runtime.Runtime runtime;
+
+ /**
+ * Bootstrap the cucumber runtime
+ *
+ * @param clazz Which has has the Cucumber and TestNG @Test annotations
+ */
+ public TestNGCucumberRunner(Class clazz) {
+
@ffbit

ffbit Nov 8, 2013

Member

Could you please remove this empty line?

@martykube

martykube Nov 9, 2013

Contributor

Sure, which empty line?

@ffbit

ffbit Nov 9, 2013

Member

line number 29.

@ffbit ffbit and 1 other commented on an outdated diff Nov 8, 2013

...in/java/cucumber/api/testng/TestNGCucumberRunner.java
+ // remove duplicates from glue path.
+ List<String> uniqueGlue = new ArrayList<String>(new HashSet<String>(runtimeOptions.getGlue()));
+ runtimeOptions.getGlue().clear();
+ runtimeOptions.getGlue().addAll(uniqueGlue);
+
+ TestNgReporter reporter = new TestNgReporter(System.out);
+ runtimeOptions.getFormatters().add(reporter);
+ ClassFinder classFinder = new ResourceLoaderClassFinder(resourceLoader, classLoader);
+ runtime = new cucumber.runtime.Runtime(resourceLoader, classFinder, classLoader, runtimeOptions);
+ }
+
+ /**
+ * Run the Cucumber features
+ */
+ public void runCukes() {
+
@ffbit

ffbit Nov 8, 2013

Member

Could you please remove this empty line?

@martykube

martykube Nov 9, 2013

Contributor

Which one?

@ffbit

ffbit Nov 9, 2013

Member

line number 51.

@ffbit ffbit commented on an outdated diff Nov 8, 2013

...amples/java/calculator/RunCukesByCompositionTest.java
@@ -0,0 +1,22 @@
+package cucumber.examples.java.calculator;
+
+import cucumber.api.CucumberOptions;
+import cucumber.api.testng.TestNGCucumberRunner;
+import org.testng.annotations.Test;
+
+/**
+ * An example of using TestNG when the test class does not inherit from
+ * AbstractTestNGCucumberTests.
+ */
+@CucumberOptions(format = "json:target/cucumber-report.json")
@ffbit

ffbit Nov 8, 2013

Member

Could you please change the report file name to something like cucumber-report-composite.json?
At the moment 2 Cucumber suites use the same file name for reporting and the second one rewrites report of the first one.

@ffbit ffbit commented on an outdated diff Nov 8, 2013

...amples/java/calculator/RunCukesByCompositionTest.java
+import cucumber.api.CucumberOptions;
+import cucumber.api.testng.TestNGCucumberRunner;
+import org.testng.annotations.Test;
+
+/**
+ * An example of using TestNG when the test class does not inherit from
+ * AbstractTestNGCucumberTests.
+ */
+@CucumberOptions(format = "json:target/cucumber-report.json")
+public class RunCukesByCompositionTest extends RunCukesByCompositionBase {
+
+ /**
+ * Create one test method that will be invoked by TestNG and invoke the
+ * Cucumber runner within that method.
+ */
+ @Test
@ffbit

ffbit Nov 8, 2013

Member

Could you please add the groups and description attributes to the @Test annotation?

Member

ffbit commented Nov 8, 2013

@martykube great job, yet again!
I'm looking forward to you changes or reasons not to do them.
Have a nice day.

@brasmusson brasmusson commented on the diff Nov 9, 2013

...in/java/cucumber/api/testng/TestNGCucumberRunner.java
+ * Bootstrap the cucumber runtime
+ *
+ * @param clazz Which has has the Cucumber and TestNG @Test annotations
+ */
+ public TestNGCucumberRunner(Class clazz) {
+
+ ClassLoader classLoader = clazz.getClassLoader();
+ ResourceLoader resourceLoader = new MultiLoader(classLoader);
+
+ RuntimeOptionsFactory runtimeOptionsFactory = new RuntimeOptionsFactory(clazz, new Class[]{CucumberOptions.class});
+ RuntimeOptions runtimeOptions = runtimeOptionsFactory.create();
+
+ // remove duplicates from glue path.
+ List<String> uniqueGlue = new ArrayList<String>(new HashSet<String>(runtimeOptions.getGlue()));
+ runtimeOptions.getGlue().clear();
+ runtimeOptions.getGlue().addAll(uniqueGlue);
@brasmusson

brasmusson Nov 9, 2013

Contributor

@martykube I reckon that you got trouble when the glue paths was not unique. Maybe it would be better if the RuntimeOptions itself should make sure that the glue paths were unique. I don't say that this should be done in this pull request, but it is something that maybe should be done as an improvement later.

@ffbit

ffbit Nov 9, 2013

Member

@brasmusson Uniqueness of glue paths is provided by line 37.

@brasmusson

brasmusson Nov 9, 2013

Contributor

@ffbit Now I do not follow. I can see that line 37-39 makes the glue paths unique. The thing is that one part of me says that that code does not belong here, because uniques of glue paths is not something that is important just for a TestNG runner, is it? But if it is important that the glue paths are unique (regardless of running from command line, using the JUnit runner, using the TestNG runner, etc), shouldn't it be the RuntimeOptions class responsibility to make sure that the glue paths are unique?

@ffbit

ffbit Nov 9, 2013

Member

@brasmusson I totally agree with you.
I think we should go much deeper to provide uniqueness of discovered classes with step definitions.
And, as you mentioned, there should be another PR on this problem.

@brasmusson thanks for your finding and I'm sorry for my bad previous comment.

@ffbit

ffbit Nov 9, 2013

Member

Just to summarise my view on the code in lines 37-39.
It's a hack, but it's a forced one. Let's let it go and fix its roots later.

@martykube

martykube Nov 9, 2013

Contributor

The trouble I had was that RuntimeOptions created duplicate entries due to the test class and the super class being in the same package. Then the test case failed due to duplicate step definitions being found during the second scan of the duplicate glue path.

I agree that this should be pushed down into RuntimeOptions.

This pull request needs the duplicate elimination. We could either 1) fix RuntimeOptions and remove the code in this pull request, or 2) Leave this patch as is and add duplicate elimination to RuntimeOptions in another pull request. Let me know what makes sense.

@ffbit

ffbit Nov 9, 2013

Member

The second option is good in my view.

Contributor

martykube commented Nov 9, 2013

@ffbit Your suggestions are good, I'll make the changes.
Two comments were for empty line removal - I'm not sure which line is the offender. Let me know, Thanks!

Member

ffbit commented Nov 10, 2013

@martykube I've just sent you a pull request with those empty lines removed. Please check it out in your own forked repo.

Contributor

martykube commented Nov 11, 2013

@ffibit got the pull for the empty lines. I'm working on the other comments and should have that finished shortly

Member

ffbit commented Nov 11, 2013

You can merge my commit and it will appear in this thread, if you want.
Just to clarify, please do not implement glue merging in this PR - it's out of scope.

11.11.2013, 23:54, "Marty Kube" notifications@github.com:

@ffibit got the pull for the empty lines. I'm working on the other comments and should have that finished shortly


Reply to this email directly or view it on GitHub.

Contributor

martykube commented Nov 11, 2013

@ffibit I've added a commit to address the comments. Let me know if there is anything else needed to allow a merge.

I was thinking about updating RuntimeOptions to make the glue paths unique.

Contributor

martykube commented Nov 12, 2013

@ffbit Your commit is in. No glue merging here :-)

I'm "hands off" on this pull request unless there are other comments,

ffbit added a commit that referenced this pull request Nov 12, 2013

Merge pull request #622 from martykube/master
Allow TestNG Cucumber runner to use composition instead of inheritance

@ffbit ffbit merged commit cca52ae into cucumber:master Nov 12, 2013

1 check passed

default The Travis CI build passed
Details
Member

ffbit commented Nov 12, 2013

@martykube
Great job!

@ffbit ffbit referenced this pull request Nov 12, 2013

Merged

History.md update on #622 #629

ffbit added a commit that referenced this pull request Nov 12, 2013

ffbit added a commit that referenced this pull request Nov 14, 2013

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