Android: Fix Cucumber execution on Gradle #1094

gnuechtel · 2017-01-24T21:51:13Z

the connected check tasks of the Android/Gradle build system do not like non-unique test names
we add a unique index to the test names if they are non-unique (e.g. on scenario outlines with multiple examples)
for this bug fix, we provide a unit test

Summary

Fix Android test execution for Gradle builds on scenario outlines with multiple examples

Details

If you would use the current implementation of Cucumber Android with the connected-check Gradle tasks plus scenario outlines and multiple examples, not all tests will be executed. With this fix, it will be executed.

Motivation and Context

This change fixes the great Cucumber software on scenario outlines with examples.
As I know, there is no open issue for this bug fix.

How Has This Been Tested?

Automatic tests have been added.

The changes have been applied to the existing Cukeulator example project on my local machine.

Screenshots (if appropriate):

Types of changes

Bug fix (non-breaking change which fixes an issue).
New feature (non-breaking change which adds functionality).
Breaking change (fix or feature that would cause existing functionality to not work as expected).

Checklist:

I've added tests for my code.
My change requires a change to the documentation.
I have updated the documentation accordingly.

* the connected check tasks of the Android/Gradle build system do not like non-unique test names * we add a unique index to the test names if they are non-unique (e.g. on scenario outlines with multiple examples) * for this bug fix, we provide a unit test

lsuski · 2017-12-07T11:41:02Z

This is very needed beacuse connectedAndroidTest task is currently unusable with Cucumber.
@cgnuechtel could you update this PR to match current version AndroidInstrumentationReporter source code?

# Conflicts: # android/src/main/java/cucumber/runtime/android/AndroidInstrumentationReporter.java # android/src/test/java/cucumber/runtime/formatter/AndroidInstrumentationReporterTest.java

* the connected check tasks of the Android/Gradle build system do not like non-unique test names * we add a unique index to the test names if they are non-unique (e.g. on scenario outlines with multiple examples) * for this bug fix, we provide a unit test This is a re-integration of PR-1094. The original pull request code was performed on 1.2.6-SNAPSHOT base. This code bases on version 2.3.2-SNAPSHOT.

coveralls · 2018-01-20T15:49:48Z

Coverage increased (+0.06%) to 81.547% when pulling 4e1a79d on cgnuechtel:master into 68c8dbf on cucumber:master.

gnuechtel · 2018-01-20T16:52:08Z

@mlvandijk the code is now up-to-date again

Please note, that this pull request only fixes ambiguous test names in Gradle HTML reports.

For a proper Android project configuration I will provide an updated version of the Cukealator project within this pull request with the next Git push.

…ools - Update to Android Studio 3.0.1, SDK 26+ and Gradle 4.1 - Replace Instrumentation class by CucumberRunner - CucumberRunner uses AndroidJUnitRunner (JUnit 4+) - CalculationSteps class uses ActivityTestRule instead of deprecated ActivityInstrumentationTestCase2 - Fix permissions to write reports on internal storage

coveralls · 2018-01-20T18:00:44Z

Coverage increased (+0.06%) to 81.544% when pulling 9b63692 on cgnuechtel:master into 68c8dbf on cucumber:master.

* Update README.md * Enable local Maven dependencies for better development experience * Describe, how to use Cukeulator example project with locally built Cucumber-JVM

gnuechtel · 2018-01-21T13:39:27Z

I created issue 1314 with some extra information how this pull request may be applied.

Please feel free to make suggestions for improvements. Maybe some code (e.g. CucumberRunner) may be transferred from the Cukeulator example project to the Cucumber Android project.

brasmusson · 2018-01-21T20:15:48Z

As the JUnit formatter already has a similar handling of making test cases names unique, I think we should use consistent behavior here, that is

if the test case name does not include a blank (" "), append "_2", "_3" etc.
if the test case name include a blank, append " 2", " 3" etc.

A rational for not using a blank in the first case, and to stay away from ( and ) in the second case is to reduce the possibility for file name issues like in #972.

…ike JUnitFormatter in cucumber-core

gnuechtel · 2018-01-21T20:42:26Z

@brasmusson Thanks for your feedback! You are right! Changed the code right now.

gnuechtel · 2018-01-29T22:52:35Z

Is anyone available to review this pull request?

mlvandijk

Unfortunately I don't use Android so cannot comment on that. Some other feedback provided below.

mlvandijk · 2018-01-30T03:55:52Z

examples/android/android-studio/Cukeulator/app/build.gradle

    androidTestCompile 'io.cucumber:cucumber-android:2.3.0'
+    //
+    // The following dependency works, if you build Cucumber-JVM on your local machine:


Should be added to the android README instead of in comments here?

@mlvandijk Thanks for your feedback

Please look at the end of README.md

There is a reference to build.gradle. I think it is better to leave the technical details in build.gradle since it does not affect most users.

mlvandijk · 2018-01-30T03:58:09Z

android/src/test/java/cucumber/runtime/formatter/AndroidInstrumentationReporterTest.java

+            .invokeGetUniqueTestNameOnDifferentStatesInTestCaseLifecycle()
+        );
+
+        // TODO Consider to split this test method into multiple ones, to fulfil the one-assert-per-test-method-paradigm


Should this be done? (or remove "TODO"). In general, PR contains a several comments that should possible either be javadoc or be removed.

@mlvandijk Thanks again for your feedback

I removed the TODO comment, added/improved comments and hopefully made the test method more readable

…oidInstrumentationReporter

lsuski · 2018-01-30T15:12:15Z

I've made some refactoring to avoid duplicating code

brasmusson · 2018-01-30T17:41:38Z

Is anyone available to review this pull request?

I will definitely not have time to look at it before the weekend.

lsuski · 2018-01-30T18:48:16Z

I've run all my Android tests with this PR but for 653 test cases got 648 reports. I need to investigate it further. Without this PR I have about 350 reports.

lsuski · 2018-01-31T15:48:08Z

I had 2 features with the same name and first of them was overwritten in gradle report. I'm not sure if feature name conflict should be somehow resolved by AndroidInstrumentationReporter or it is just bad practice and should be avoided. For me this PR can be merged as is.

brasmusson

I apologize for delayed review.

The behavior works, but I think that getUniqueTestName and ensureUniqueTestName should be made into one method, and that the test should be divided so that the test names provides specification/documentation of the behavior under test.

brasmusson · 2018-02-04T20:12:20Z

android/src/test/java/cucumber/runtime/formatter/AndroidInstrumentationReporterTest.java

+     * More technical: Checks, if {{@link AndroidInstrumentationReporter#getUniqueTestName(TestCase)}} is working.
+     *
+     * Note about the test code: We are using multiple assertions and for-loops in this method, which is a code smell.
+     * Here it is okay, since we are just answering the question if the getUniqueTestName-method is working or not.


I'm a big fan of "Test method names should be sentences (that specify/document the item under test)" (see https://dannorth.net/introducing-bdd/), so I do not think this test is okay. The interesting behavior that should be specified/documented in test method names includes:

test case names are made unique with numbered suffices

the numbered suffices do not include blanks when test case names do not include blanks

out of order identical test case names are also made unique

running a test case several times will use the same name each time

identical test case names in different feature will not be changed

brasmusson · 2018-02-04T20:22:42Z

android/src/main/java/cucumber/runtime/formatter/AndroidInstrumentationReporter.java

-        currentTestCaseName = testCase.getName();
+        // Since the names of test cases are not guaranteed to be unique, we must check for unique names
+        ensureUniqueTestName(testCase);
+        currentTestCaseName = getUniqueTestName(testCase);


What we want to do here is to calculate the unique name for the current test case, to divide this into two methods which are dependent and need to be called in order i s IMHO not the right solution (if this is driven from "command query separation", I do not think it is the right place for it). I think getUniqueTestName (maybe renamed calculateUniqueTestName to indicate that it is not just a getter) should both do the work and return the result.

brasmusson · 2018-02-04T20:30:36Z

android/src/main/java/cucumber/runtime/formatter/AndroidInstrumentationReporter.java

+     * @param testCase the testCase
+     * @return the unique test name or {@code null}
+     */
+    String getUniqueTestName(TestCase testCase) {


If this method did all the work I could be okay with it being package private and all details of its behavior tested by direct calls from the test class. Currently when its only use in test is to fetch data after startTestCase has been executed from the tests, I think it should be made private and the tests get the test names from the instrumentation (like in this test)

brasmusson · 2018-02-04T20:33:00Z

android/src/test/java/cucumber/runtime/formatter/AndroidInstrumentationReporterTest.java

@@ -458,6 +464,128 @@ public void step_result_contains_only_the_current_scenarios_severest_result() {
        inOrder.verify(instrumentation).sendStatus(eq(StatusCodes.OK), secondCaptor.capture());
    }

+    /**
+     * Verifies, that different test names are used when scenario names are duplicated.
+     * More technical: Checks, if {{@link AndroidInstrumentationReporter#getUniqueTestName(TestCase)}} is working.


Currently, the behavior tested is performed by ensureUniqueTestName (but I'm suggesting to change that).

brasmusson · 2018-02-04T20:36:53Z

android/src/test/java/cucumber/runtime/formatter/AndroidInstrumentationReporterTest.java

+        }
+
+        // when
+        private void invokeGetUniqueTestNameOnDifferentTestCaseStates() {


This is just confusing. The unique name is calculated once (in startTestCase), so calling getUniqueTestName several times during the test life cycle cannot be relevant. This is a good reason why getUniqueTestName should be private.

@brasmusson Thanks for your review. It was very helpful

I agree with your point, that my tests are too low-level, will correct it later

brasmusson · 2018-02-04T20:38:08Z

android/src/main/java/cucumber/runtime/formatter/AndroidInstrumentationReporter.java

+    /**
+     * Creates a unique test name for the given test case by filling the internal maps
+     * {@link #uniqueTestNameForTestCase} and {@link #uniqueTestNamesForFeature}.<br/>
+     * If the test case name is unique, it will be used, otherwise, a index will be added "(2)", "(3)", "(4)", ...


"(2)" etc, is not correct anymore.

brasmusson · 2018-02-04T20:41:54Z

android/src/main/java/cucumber/runtime/formatter/AndroidInstrumentationReporter.java

+
+    /**
+     * Gets the unique test name for the given test case.<br/>
+     * If, {@link #ensureUniqueTestName(TestCase)} was not yet invoked for the test case, {@code null} will be returned.


This indicate the need to make a check for null when calling getUniqueTestName but it should not be used in such a way that will happen. For me it is another reason why getUniqueTestName and ensureUniqueTestName should be made into one method.

- Create merged method calculateUniqueTestName from getUniqueTestName and ensureUniqueTestName and make it private - Use better test method name: test_case_names_are_unique_on_equal_scenario_names (instead of scenario_outline_all_test_names_unique) - Refactor test code: now it should be readable

gnuechtel · 2018-02-05T22:22:25Z

@brasmusson Your suggestions should be implemented now. Thanks again!

The test method is still a little bit long with some for-loops, but I think that should be okay here.
Before it was unreadable. Hopefully it is usable now.

gnuechtel · 2018-02-14T13:47:49Z

@brasmusson Can you look again at this pull request, please?

To provide better documentation of the functionality, that is: * test names within feature are made unique by appending blank and number * test names within are made unique by appending underscore and number when no blank in name * test names in different features can be the same * test names are made unique also when not consecutive

brasmusson · 2018-02-18T19:22:16Z

@cgnuechtel I was not clear enough that I wanted the test case split up. I fixed that in 9b63692, now the test case names provide the documentation:

test names within feature are made unique by appending blank and
number
test names within are made unique by appending underscore and number
when no blank in name
test names in different features can be the same
test names are made unique also when not consecutive

aslakhellesoy · 2018-02-18T19:23:12Z

Hi @cgnuechtel,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber

[SKIP CI]

gnuechtel · 2018-02-19T18:38:07Z

@brasmusson and @aslakhellesoy

Thanks for the good news 😊

lock · 2019-02-19T18:50:13Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

mpkorstanje force-pushed the master branch from ad3e012 to fdee635 Compare October 12, 2017 16:17

mpkorstanje force-pushed the master branch 3 times, most recently from 4e47cec to acb4bb5 Compare December 4, 2017 22:28

mpkorstanje force-pushed the master branch 6 times, most recently from 159ec4c to 74325ea Compare December 10, 2017 20:05

mlvandijk added the Android label Dec 12, 2017

gnuechtel added 2 commits January 20, 2018 15:52

Merge branch 'master' of https://github.com/cgnuechtel/cucumber-jvm

5d01949

# Conflicts: # android/src/main/java/cucumber/runtime/android/AndroidInstrumentationReporter.java # android/src/test/java/cucumber/runtime/formatter/AndroidInstrumentationReporterTest.java

gnuechtel changed the title ~~Fix AndroidInstrumentationReporter for Gradle builds~~ Fix Cucumber execution on Gradle Jan 20, 2018

Improve Cukeulator example project

f7dba2a

* Update README.md * Enable local Maven dependencies for better development experience * Describe, how to use Cukeulator example project with locally built Cucumber-JVM

gnuechtel mentioned this pull request Jan 21, 2018

Android: Cucumber execution does not work on Gradle #1314

Closed

gnuechtel changed the title ~~Fix Cucumber execution on Gradle~~ Android: Fix Cucumber execution on Gradle Jan 21, 2018

Rename duplicated test case names in AndroidInstrumentationReporter l…

da8da77

…ike JUnitFormatter in cucumber-core

Fix typo

a512d20

mlvandijk added the Bug label Jan 30, 2018

mlvandijk reviewed Jan 30, 2018

View reviewed changes

gnuechtel and others added 2 commits January 30, 2018 10:39

Improve readability of AndroidInstrumentationReporterTest

d37ed34

Share common logic for test case name between JUnitFormatter and Andr…

7e26df4

…oidInstrumentationReporter

brasmusson requested changes Feb 4, 2018

View reviewed changes

Change misleading variable names

9181387

brasmusson approved these changes Feb 18, 2018

View reviewed changes

brasmusson merged commit d8406dd into cucumber:master Feb 18, 2018

brasmusson added a commit that referenced this pull request Feb 18, 2018

Attribution #1094.

436c5d4

[SKIP CI]

lock bot locked as resolved and limited conversation to collaborators Feb 19, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Android: Fix Cucumber execution on Gradle #1094

Android: Fix Cucumber execution on Gradle #1094

gnuechtel commented Jan 24, 2017

lsuski commented Dec 7, 2017

coveralls commented Jan 20, 2018

gnuechtel commented Jan 20, 2018 •

edited

Loading

coveralls commented Jan 20, 2018 •

edited

Loading

gnuechtel commented Jan 21, 2018

brasmusson commented Jan 21, 2018

gnuechtel commented Jan 21, 2018

gnuechtel commented Jan 29, 2018

mlvandijk left a comment •

edited

Loading

mlvandijk Jan 30, 2018

gnuechtel Jan 30, 2018

mlvandijk Jan 30, 2018

gnuechtel Jan 30, 2018

lsuski commented Jan 30, 2018

brasmusson commented Jan 30, 2018

lsuski commented Jan 30, 2018

lsuski commented Jan 31, 2018

brasmusson left a comment

brasmusson Feb 4, 2018

brasmusson Feb 4, 2018

brasmusson Feb 4, 2018

brasmusson Feb 4, 2018

brasmusson Feb 4, 2018

gnuechtel Feb 5, 2018

brasmusson Feb 4, 2018

brasmusson Feb 4, 2018

gnuechtel commented Feb 5, 2018

gnuechtel commented Feb 14, 2018

brasmusson commented Feb 18, 2018

aslakhellesoy commented Feb 18, 2018

gnuechtel commented Feb 19, 2018

lock bot commented Feb 19, 2019

Android: Fix Cucumber execution on Gradle #1094

Android: Fix Cucumber execution on Gradle #1094

Conversation

gnuechtel commented Jan 24, 2017

Summary

Details

Motivation and Context

How Has This Been Tested?

Screenshots (if appropriate):

Types of changes

Checklist:

lsuski commented Dec 7, 2017

coveralls commented Jan 20, 2018

gnuechtel commented Jan 20, 2018 • edited Loading

coveralls commented Jan 20, 2018 • edited Loading

gnuechtel commented Jan 21, 2018

brasmusson commented Jan 21, 2018

gnuechtel commented Jan 21, 2018

gnuechtel commented Jan 29, 2018

mlvandijk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lsuski commented Jan 30, 2018

brasmusson commented Jan 30, 2018

lsuski commented Jan 30, 2018

lsuski commented Jan 31, 2018

brasmusson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gnuechtel commented Feb 5, 2018

gnuechtel commented Feb 14, 2018

brasmusson commented Feb 18, 2018

aslakhellesoy commented Feb 18, 2018

gnuechtel commented Feb 19, 2018

lock bot commented Feb 19, 2019

gnuechtel commented Jan 20, 2018 •

edited

Loading

coveralls commented Jan 20, 2018 •

edited

Loading

mlvandijk left a comment •

edited

Loading