Issue 236, Extended Description argument parsing functionality #283

cirquit · 2016-12-28T11:53:38Z

added front-end parsing for extended descriptions with arguments
added front-end tests in utilTest.js
added example usage in ExtendedDescriptionTest.java
added example for @as usage with direct references to arguments in AsAnnotationExampleTest.java
added documentation for @as and @ExtendedDescription in step-usage

missing:

tests for extended description in html5report

Airblader

Haven't looked at this in detail, just a quick glance

Airblader · 2016-12-28T11:54:28Z

...-examples/src/test/java/com/tngtech/jgiven/examples/description/AsAnnotationExampleTest.java

@@ -27,6 +30,15 @@ public void scenarios_can_have_an_extended_description() {
            .and().something();
    }

+    @Test
+    @As( "Scenario that shows the usage of @At with argument enumeration")


Airblader · 2016-12-28T11:56:26Z

...examples/src/test/java/com/tngtech/jgiven/examples/description/ExtendedDescriptionsTest.java

+            return this;
+        }
+
+        @ExtendedDescription( "We can reference the first argument with $$ or $$1 : $" )


Why do we want this?

This was proposed in Isse236, I double escaped the first two occurrences to view the message in the report, but maybe this is misleading

janschaefer · 2016-12-28T12:24:51Z

As discussed offline it would be great to also support substitution by parameter name like $foo

janschaefer · 2017-01-01T11:06:14Z

@cirquit I have finished the work on the HTML App extraction. This means, however, that you have to apply your changes to the new code. Sorry about that! On the good side: lodash is the newest version!

See #287 for details.

janschaefer · 2017-01-04T21:04:43Z

...examples/src/test/java/com/tngtech/jgiven/examples/description/ExtendedDescriptionsTest.java

+import org.junit.Test;
+import org.junit.runner.RunWith;
+
+@RunWith( JUnitParamsRunner.class )


We typically use the DataProviderRunner for parameterized tests

janschaefer · 2017-01-04T21:07:52Z

...examples/src/test/java/com/tngtech/jgiven/examples/description/ExtendedDescriptionsTest.java

+    })
+    public void scenarios_with_multiple_argument_parameters_can_be_shown_via_click_on_table( boolean bool, int i ) {
+        given().some_bool_$_and_int_$_value( bool, i );
+        // TODO screenshot test & implement frontend "click on table"


Please move this test to the jgiven-tests project, as the examples project is not meant for 'real' tests.

This will be a example for future use, so I would propose to let this be and add a test after the feature is implemented.

janschaefer · 2017-01-04T21:10:25Z

jgiven-html-app/src/js/util.js

+
+function getArgumentList(wordList) {
+  function isArgument(word) { return !!word.argumentInfo; }
+  return _.chain(wordList)


could you please check whether the use of the chain method has increased the artifact size? There seem to be a problem with that function: https://medium.com/making-internets/why-using-chain-is-a-mistake-9bc1f80d51ba#.4s9828vud

janschaefer · 2017-01-04T21:14:40Z

jgiven-html-app/src/js/util.js

+    var placeHolderCount = 0;
+
+    for(var i = 0; i < string.length; ++i) {
+        var c           = string.charAt(i);


I am not a big fan of aligning code vertically. I typically automatically format my code, so this hand-crafted formatting will be destroyed the next time I touch the file. I guess we need a coding style guide for the JavaScript code :-)

janschaefer · 2017-01-04T21:16:13Z

...-examples/src/test/java/com/tngtech/jgiven/examples/description/AsAnnotationExampleTest.java


+@RunWith( JUnitParamsRunner.class )


Please use the DataProviderRunner instead

janschaefer · 2017-01-04T22:02:45Z

I think I found a general problem. Actually, JGiven does not really implement placeholder variables by name. The current behavior is simply that you can give a placeholder an arbitrary name. But the name is only relevant if it is a number. So if you have something like

@As("Second: $second, First: $first)
    public AsAnnotationStage argument_names_cannot_be_referenced( int $first, int $second ) {
}

and you call the method like argument_names_cannot_be_referenced( 1, 2 ) will result in

Second: 1, First: 2

while this first sounds wrong, however, the basic idea behind that is that you can give your placeholders arbitrary names. In practice, it also makes nearly never sense to change the order of the arguments. However, I still think that this would be useful feature. The problem now is that changing the behavior could break existing code.

For now I suggest that you leave your implementation as it is is, but only document the feature for the @ExtendedDescription annotation and not the @as annotation.

janschaefer · 2017-01-04T21:26:08Z

jgiven-html-app/src/js/util.js

@@ -2,6 +2,7 @@
 * Utility functions
 */

+let _ = require('lodash');


please import 'lodash/core' to reduce the final artifact size. You can also use the ES6 syntax:

import _ from 'lodash/core'

janschaefer · 2017-01-04T22:13:08Z

In a next step I suggest to implement the feature for the @As annotation. I think the likelihood that this change will breaking anybodies existing code is very low. As it would mean that someone has used the name of a parameter at a different position and still the value of the parameter of the original position was used.

cirquit · 2017-01-05T02:00:40Z

There were some non trivial problems while using webpack with lodash on my side. Based on how I imported the functions, some had a different behaviour and some tests broke.

import takeWhile from 'lodash/fp/takeWhile'
import drop from ...
...

vs

import _ from 'lodash/core'

vs

import _ from 'lodash'

in util.js.

The custom import broke almost every function except flow, forEach, curry and filter. The test ran through as if the functions were there, but their behaviour was not as specified (predicates didn't work, drop didn't drop enough etc.)...this is very disturbing.

Your proposed import solution resulted in a test failure, because of the TypeError undefined is not a constructor (evaluating '_core2.default.functionname...' in utilTest.js)

I'm trying to get onto why the custom import of functions does not behave the way it should.

And for the new @as functionality, I'm on board with implementing the references by argument names.

cirquit · 2017-01-05T02:08:10Z

Something is kind of strange in the util.js, because the function deselectAll seemed to work without ever importing lodash or the _ object.

I've also stumbled upon the old code in jgiven-html5-report src/test/app/*.js, should this be removed?

cirquit · 2017-01-05T03:33:16Z

I have now a working test where both imported functions from lodash (one with _, and one without) which result in different outputs. The example is the indexOf function.

import indexOf from 'lodash/fp';
import _ from 'lodash';

function parseNextArgumentName(string) {
    var stopChars = [" ", ",", ";", ":","\"","%","!","[","]","(",")","-","_"];
    var isNonStopChar = function (c) {
        console.log("char: " + c + ", _.ix: " + _.indexOf(stopChars, c)
                                 + ", ix: " + indexOf(stopChars, c));
        return indexOf(stopChars, c) === -1;
     };
    var res = _.takeWhile(string, isNonStopChar).join("");
    return res;
}

The call to parseNextArgumentName('i, bool : $bool')results in:

LOG: 'char: i, _.ix: -1, ix: -1'
LOG: 'char: ,, _.ix: 1, ix: -1'
LOG: 'char:  , _.ix: 0, ix: -1'
LOG: 'char: b, _.ix: -1, ix: -1'
LOG: 'char: o, _.ix: -1, ix: -1'
LOG: 'char: o, _.ix: -1, ix: -1'
LOG: 'char: l, _.ix: -1, ix: -1'
LOG: 'char:  , _.ix: 0, ix: -1'
LOG: 'char: :, _.ix: 3, ix: -1'
LOG: 'char:  , _.ix: 0, ix: -1'
LOG: 'char: $, _.ix: -1, ix: -1'
LOG: 'char: b, _.ix: -1, ix: -1'
LOG: 'char: o, _.ix: -1, ix: -1'
LOG: 'char: o, _.ix: -1, ix: -1'
LOG: 'char: l, _.ix: -1, ix: -1'

The manually importedindexOffunction can't find the comma in the array, but the automatically imported one can...

janschaefer · 2017-01-05T06:36:35Z

The point is that you import from the 'fp' module of lodash. The functions of this module are different. I would suggest to not use this for now to avoid these kinds of problems.

janschaefer · 2017-01-05T07:58:38Z

Regarding

I've also stumbled upon the old code in jgiven-html5-report src/test/app/*.js, should this be removed?

I have removed them on master now.

janschaefer · 2017-01-05T08:03:23Z

There is another minor issue regarding consistency between the @As implementation and your implementation, regarding spaces after placeholders. In JGiven it is currently not possible to not have a space after a placeholder. This means that something like @As('foo, $1, baz') where $1 is replaced with bar will result in foo, bar , baz. JGiven will currently always create a space after a placeholder. This has historic reasons :-). But, I also suggest that for the @ExtendedDescription case you should implement it as expected, namely without the space and we try to fix it later for the @As annotation.

cirquit · 2017-01-05T17:30:46Z

I fixed the imports now, this is still a very strange behaviour of the flowfunction, because it is implemented the same way as in lodash/fp regarding the argument order of the inner functions. One needs to use partial almost for every library function that takes the arguments in the following order ([collection], function).

Edit: Currently working on the real test to check the html for the valid extended description.

cirquit · 2017-01-11T13:59:15Z

I think I found another error, since the merge to the new jgiven-html-app the tests in HtmlReport don't work anymore. I tested it on my issue branch and the branch where the new html-app was introduced.

The problem seems to be this, and with my little knowledge of angular it might be that this call to filter:

jgivenReportApp.filter('encodeUri', function ($window) {
    return $window.encodeURIComponent;
})

in reportApp.js is the culprit.

With the previous minor version of angular-1.4.8 instead of 1.5.8 only one of the tests fails, namely this one, Line 96 . The tested functionality works if tested manually, but this test throws an error because of the WebDriver, that can't find any tags.

The error is thrown here in the findTagWithName method. After changing By.xpath to a simplified By.linkText it found the tag, but the click() didn't work and the title of the scenario was still All Scenarios instead of the [TagName].

This may be still the problem of angular.

Edit: Interestingly enough, an implicitlyWait with 10 ms fixes this test. Seems that the WebDriver tried to click the link before it's clickable. Unfortunately this increases the test time up to 3+ minutes and not a really viable solution.

Edit 2: This problem resolved itself after updating to the current master branch. Added the current sometimes working version.

@as

- added front-end tests in utilTest.js - added example usage in ExtendedDescriptionTest.java - added example for @as usage with direct references to arguments in AsAnnotationExampleTest.java - added documentation for @as and @ExtendedDescription in step-usage missing: - tests for extended description in html5report

fixed typo

* added js unit tests * modified docs to show named arguments

cirquit · 2017-01-18T12:30:28Z

Looks like this is only a problem on my ubuntu box:

Ubuntu 15.10 wiliy
npm 2.14.12
yarn 0.18.1
oracle javac 1.7.0_80

The mac, as well as travis don't seem to have any issues.

janschaefer · 2017-01-18T12:42:05Z

Ok. Then ignore the issues on your machine for now :-)

function from util.js

janschaefer · 2017-01-19T13:12:31Z

jgiven-tests/src/test/java/com/tngtech/jgiven/report/html5/Html5AppTest.java


        given().a_report_model()
                .and().step_$_of_scenario_$_has_extended_description_with_arguments( 1, 1, description, argumentMap );
        jsonReports
                .and().the_report_exist_as_JSON_file();
        whenReport.when().the_HTML_Report_Generator_is_executed();
-        when().the_page_of_scenario_$_is_opened( 1 );
+        when().the_page_of_scenario_$_is_opened( 1 )
+              .and().show_angular_foundation_tooltip_with_$_ms_delay_for_element_$_with_attribute_$_and_value_$(251, "span", "class", "has-tip" );


You should introduce another step here that hides the internal details of the implementation. Always think from the perspective of a non-technical person, when writing the scenarios.
For example: the_tooltip_of_extended_description_is_shown(). That method can then call your internal implementation.

I thought of it as the person looking at the test, and if it didn't work because of the newly introduced framework that doesn't use angular-tooltips, I would want to know this dependency explicitly.

janschaefer · 2017-01-19T13:14:43Z

jgiven-tests/src/test/java/com/tngtech/jgiven/report/html5/WhenHtml5App.java

+        WebElement webElement = webDriver.findElements( By.xpath( "//" + element + "[@"+ attribute +"='" + value + "']" )).get( 0 );
+        builder.moveToElement( webElement ).clickAndHold().build().perform();
+        WebDriverWait wait = new WebDriverWait( webDriver, ms );
+        wait.until( ExpectedConditions.presenceOfElementLocated( By.xpath( "//span[@is-open]" )));


ok, that is nice!

Airblader reviewed Dec 28, 2016

View reviewed changes

cirquit force-pushed the issue236 branch from 6029408 to 12ec238 Compare January 3, 2017 15:16

janschaefer reviewed Jan 4, 2017

View reviewed changes

cirquit added 10 commits January 11, 2017 16:36

fixed typo

4c61972

fixed typo

updated previous work for webpack

c793274

* added named argument parsing for extended descriptions

8bff054

* added js unit tests * modified docs to show named arguments

* fixed minor js errors, added a test

a42b598

* refactored _.chain, tried to manage imports

a533ba7

changed from JUnitParamsRunner to DataProviderRunner

ebcf94e

removed named references from docs

4bdcd29

fixed imports, removed utilTest from old html5Report

edcf20c

updated current _sometimes_ working state

49f04c5

cirquit force-pushed the issue236 branch from 802279c to 49f04c5 Compare January 11, 2017 18:25

Added selenium test for extended description, removed unnecessary flow

d012d57

function from util.js

janschaefer suggested changes Jan 19, 2017

View reviewed changes

refactored show-tooltip function to be more descriptive

2337793

janschaefer merged commit 3f1c5a6 into TNG:master Jan 20, 2017

janschaefer added core HTML App enhancement labels Jan 20, 2017

janschaefer added this to the v0.15.0 milestone Jan 20, 2017

janschaefer mentioned this pull request Jan 20, 2017

cannot use $, $1 , etc... in ExtendedDescription #236

Closed

cirquit deleted the issue236 branch January 20, 2017 21:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 236, Extended Description argument parsing functionality #283

Issue 236, Extended Description argument parsing functionality #283

cirquit commented Dec 28, 2016

Airblader left a comment

Airblader Dec 28, 2016

Airblader Dec 28, 2016

cirquit Dec 28, 2016

janschaefer commented Dec 28, 2016

janschaefer commented Jan 1, 2017 •

edited

Loading

janschaefer Jan 4, 2017

janschaefer Jan 4, 2017

cirquit Jan 5, 2017

janschaefer Jan 4, 2017

janschaefer Jan 4, 2017

janschaefer Jan 4, 2017

janschaefer commented Jan 4, 2017

janschaefer Jan 4, 2017

janschaefer commented Jan 4, 2017

cirquit commented Jan 5, 2017

cirquit commented Jan 5, 2017

cirquit commented Jan 5, 2017

janschaefer commented Jan 5, 2017

janschaefer commented Jan 5, 2017

janschaefer commented Jan 5, 2017 •

edited

Loading

cirquit commented Jan 5, 2017 •

edited

Loading

cirquit commented Jan 11, 2017 •

edited

Loading

cirquit commented Jan 18, 2017

janschaefer commented Jan 18, 2017

janschaefer Jan 19, 2017

cirquit Jan 19, 2017

janschaefer Jan 19, 2017


		@RunWith( JUnitParamsRunner.class )

Issue 236, Extended Description argument parsing functionality #283

Issue 236, Extended Description argument parsing functionality #283

Conversation

cirquit commented Dec 28, 2016

Airblader 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

janschaefer commented Dec 28, 2016

janschaefer commented Jan 1, 2017 • 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

Choose a reason for hiding this comment

janschaefer commented Jan 4, 2017

Choose a reason for hiding this comment

janschaefer commented Jan 4, 2017

cirquit commented Jan 5, 2017

cirquit commented Jan 5, 2017

cirquit commented Jan 5, 2017

janschaefer commented Jan 5, 2017

janschaefer commented Jan 5, 2017

janschaefer commented Jan 5, 2017 • edited Loading

cirquit commented Jan 5, 2017 • edited Loading

cirquit commented Jan 11, 2017 • edited Loading

cirquit commented Jan 18, 2017

janschaefer commented Jan 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

janschaefer commented Jan 1, 2017 •

edited

Loading

janschaefer commented Jan 5, 2017 •

edited

Loading

cirquit commented Jan 5, 2017 •

edited

Loading

cirquit commented Jan 11, 2017 •

edited

Loading