[Core] Add --wip option #1381

Heziode · 2018-06-02T14:06:51Z

Summary

This feature is the java implementation of --wip of Cucumber-Ruby. But, instead of not support --wip with --strict (in Ruby version), this version do it

How Has This Been Tested?

I have do tests in:

Result
RuntimeOptions
Runtime

Test in ExitStatus are not relevant here because it is tested in Runtime, isn't it ?

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.

mpkorstanje · 2018-06-02T15:26:01Z

core/src/main/java/cucumber/runtime/ExitStatus.java

@@ -24,7 +25,8 @@ public void receive(TestCaseFinished event) {
        }
    };

-    ExitStatus() {
+    ExitStatus(boolean wip) {


Both wip and strict are derived from RuntimeOptions. It is okay to inject the whole runtime options here.

Add whole RuntimeOptions

mpkorstanje · 2018-06-02T15:36:34Z

core/src/main/java/cucumber/api/Result.java

-    private boolean hasAlwaysOkStatus() {
-        return is(Result.Type.PASSED) || is(Result.Type.SKIPPED);
+    public boolean isOkWip(boolean isStrict) {
+        return hasAlwaysOkStatus(true) || !isStrict && hasOkWhenNotStrictStatus();


I don't think this method is needed.

When using --wip the build should fail when there are any passing results (when result.isOk(true/false) return true).

When using --wip the build should fail when there are any passing results (when result.isOk(true/false) return true).

Are you sure ?
Because if I do this, when it return false for undefined in strict mode I attempt to obtain result 1 but I get 0 (with my current code, that are not currently pushed)

@mpkorstanje If I only use isOk, and I do what you tell, I obtain an unexpected behaviour with strict mode.

With wip and strict, if I have an undefined step, the result is 0 instead of 1.

I thought it wasn't possible to use wip and strict in together. But if it is, that makes sense. An undefined result in strict mode makes the test fail. So when running with wip and strict it should pass. That is what I get from reading work_in_progress.feature:

In order to ensure that feature scenarios do not pass until they are expected to Developers should be able to run cucumber in a mode that - will fail if any scenario passes completely - will not fail otherwise

Ok, do you want I disable the use of strict with wip ?

Why do you think the behaviour is unexpected?

I mentioned somewhere else that I couldn’t remember why we disallowe strict and wip at the same time.

If we can’t think of a good reason for it (I can’t) we should allow it.

I am thinking the same thing. But I am trying to work out why Heziode thinks the result of combining wip and strict with an undefined result is unexpected.

Here is a result I want to expect:

--wip: Test fail only if one or more scenario have status PASSED

--wip + --strict: Test fail if one or more scenario have status different than FAILED

What do you think about this ?

mpkorstanje · 2018-06-02T15:40:10Z

core/src/main/java/cucumber/runtime/ExitStatus.java

+    public byte exitStatus() {
+        return results.isEmpty() ||
+            (runtimeOptions.isWip() ? max(results, SEVERITY).isOkWip(runtimeOptions.isStrict()) :
+                max(results, SEVERITY).isOk(runtimeOptions.isStrict())) ? 0x0 : ERRORS;


It's okay to use if statements here. You can use guard clauses to handle the empty case and the wip case.

I also think this will fail when there is one test case that passes and one test case that failed.

Can you add a test case to prove it works?

Yep, I can add test to check this, but… where I write it ?

In runtime test. But you have to test with moire then one event on the bus.

It is (normally) fixed with 99e535e

Add --wip option

mpkorstanje · 2018-06-03T22:59:50Z

Well I made a table. I used the work_in_progress.feature for the --wip, --no-strict column, the --no-wip columns are how cucumber currently works. The --wip --strict column is just the two obvious ones.

           |       --wip            |       --no-wip         |
           | --strict | --no-strict | --strict | --no-strict | 
PASSED     |   fail   |  fail       |   pass   |  pass       |
SKIPPED    |    ?     |  ?          |   pass   |  pass       |
PENDING    |    ?     |  pass       |   fail   |  pass       |
UNDEFINED  |    ?     |  pass       |   fail   |  pass       |
AMBIGUOUS  |    ?     |  pass       |   fail   |  fail       |
FAILED     |  pass    |  pass       |   fail   |  fail       |

The question mark at --wip --no-strict is okay because as far as I know you can't skip steps in ruby without making a prior step fail.

You can however skip steps in java using Assume. So we have two options to fill out that one question mark:

A:

           |       --wip            |       --no-wip         |
           | --strict | --no-strict | --strict | --no-strict | 
PASSED     |   fail   |  fail       |   pass   |  pass       |
SKIPPED    |    ?     |  fail       |   pass   |  pass       |
PENDING    |    ?     |  pass       |   fail   |  pass       |
UNDEFINED  |    ?     |  pass       |   fail   |  pass       |
AMBIGUOUS  |    ?     |  pass       |   fail   |  fail       |
FAILED     |  pass    |  pass       |   fail   |  fail       |

Interesting here is that --wip --no-strict is the inverse of --no-wip --strict. Which would explain why wip and strict modes can't be used together. The wip mode is already strict (and inversed).

B:

           |       --wip            |       --no-wip         |
           | --strict | --no-strict | --strict | --no-strict | 
PASSED     |   fail   |  fail       |   pass   |  pass       |
SKIPPED    |    ?     |  pass       |   pass   |  pass       |
PENDING    |    ?     |  pass       |   fail   |  pass       |
UNDEFINED  |    ?     |  pass       |   fail   |  pass       |
AMBIGUOUS  |    ?     |  pass       |   fail   |  fail       |
FAILED     |  pass    |  pass       |   fail   |  fail       |

You can also make an argument for this. When work is in progress you wouldn't expect a the build to fail because you made a scenario pass by skipping it entirely.

And again the wip mode more stricter then strict (and inverted).

--wip + --strict: Test fail if one or more scenario have status different than FAILED

That would also be a logical alternative.

B2:

           |       --wip            |       --no-wip         |
           | --strict | --no-strict | --strict | --no-strict | 
PASSED     |   fail   |  fail       |   pass   |  pass       |
SKIPPED    |   fail   |  pass       |   pass   |  pass       |
PENDING    |   fail   |  pass       |   fail   |  pass       |
UNDEFINED  |   fail   |  pass       |   fail   |  pass       |
AMBIGUOUS  |   fail   |  pass       |   fail   |  fail       |
FAILED     |   pass   |  pass       |   fail   |  fail       |

@Heziode What use case did you have in mind here?

@aslakhellesoy what kind of work flow was wip supposed to support anyway?

Heziode · 2018-06-04T04:54:31Z

@mpkorstanje The option B2 is what I expect. But, in what use case use --strict with --wip, I do not know ...

mpkorstanje · 2018-06-04T18:15:31Z

Let's go for option B2 then. Basically just ignore strict when wip is active.

           |       --wip            |       --no-wip         |
           | --strict | --no-strict | --strict | --no-strict | 
PASSED     |   fail   |  fail       |   pass   |  pass       |
SKIPPED    |   pass   |  pass       |   pass   |  pass       |
PENDING    |   pass   |  pass       |   fail   |  pass       |
UNDEFINED  |   pass   |  pass       |   fail   |  pass       |
AMBIGUOUS  |   pass   |  pass       |   fail   |  fail       |
FAILED     |   pass   |  pass       |   fail   |  fail       |

A very concise way to check if the least severe result is a passed would be: min(results, SEVERITY).is(Type.PASSED); so the wip methods in Result aren't needed.

Heziode · 2018-06-04T19:54:07Z

It is done.

mpkorstanje · 2018-06-05T13:42:44Z

Looks good. I'll try to merge this and the runtime constructors branch later this week.

mpkorstanje · 2018-06-05T16:55:25Z

Alright. I had some spare time. It has been merged.

Thanks for contributing!

lock · 2019-06-05T18:10:40Z

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.

feat(core): Add --wip option

9097125

Heziode changed the title ~~feat(core): Add --wip option~~ Add --wip option Jun 2, 2018

mpkorstanje reviewed Jun 2, 2018

View reviewed changes

feat(core): Improve exitStatus use

c7bc9e0

Add whole RuntimeOptions

mpkorstanje reviewed Jun 2, 2018

View reviewed changes

Heziode added 2 commits June 3, 2018 00:31

feat(core): Fix trouble with exitStatus

99e535e

feat(core): Update USAGE.txt

3f19001

Add --wip option

mpkorstanje changed the title ~~Add --wip option~~ [Core] Add --wip option Jun 5, 2018

Merge branch 'refactor-runtime-constructors' into feature/wip-option

3f361d2

mpkorstanje closed this in 6a2ca5e Jun 5, 2018

Heziode deleted the feature/wip-option branch June 5, 2018 17:41

mpkorstanje mentioned this pull request Oct 30, 2018

Proposal: --wip-tags TAG_EXPRESSION cucumber/common#519

Closed

lock bot locked as resolved and limited conversation to collaborators Jun 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Core] Add --wip option #1381

[Core] Add --wip option #1381

Heziode commented Jun 2, 2018

mpkorstanje Jun 2, 2018

Heziode Jun 2, 2018

mpkorstanje Jun 2, 2018

Heziode Jun 2, 2018 •

edited

Loading

Heziode Jun 2, 2018

mpkorstanje Jun 3, 2018

Heziode Jun 3, 2018

mpkorstanje Jun 3, 2018

aslakhellesoy Jun 3, 2018

mpkorstanje Jun 3, 2018

Heziode Jun 3, 2018 •

edited

Loading

mpkorstanje Jun 2, 2018 •

edited

Loading

mpkorstanje Jun 2, 2018

Heziode Jun 2, 2018

mpkorstanje Jun 2, 2018 •

edited

Loading

Heziode Jun 2, 2018

mpkorstanje commented Jun 3, 2018 •

edited

Loading

Heziode commented Jun 4, 2018 •

edited

Loading

mpkorstanje commented Jun 4, 2018 •

edited

Loading

Heziode commented Jun 4, 2018

mpkorstanje commented Jun 5, 2018

mpkorstanje commented Jun 5, 2018

lock bot commented Jun 5, 2019

[Core] Add --wip option #1381

[Core] Add --wip option #1381

Conversation

Heziode commented Jun 2, 2018

Summary

How Has This Been Tested?

Types of changes

Checklist:

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Heziode Jun 2, 2018 • 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

Choose a reason for hiding this comment

Heziode Jun 3, 2018 • edited Loading

Choose a reason for hiding this comment

mpkorstanje Jun 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpkorstanje Jun 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mpkorstanje commented Jun 3, 2018 • edited Loading

Heziode commented Jun 4, 2018 • edited Loading

mpkorstanje commented Jun 4, 2018 • edited Loading

Heziode commented Jun 4, 2018

mpkorstanje commented Jun 5, 2018

mpkorstanje commented Jun 5, 2018

lock bot commented Jun 5, 2019

Heziode Jun 2, 2018 •

edited

Loading

Heziode Jun 3, 2018 •

edited

Loading

mpkorstanje Jun 2, 2018 •

edited

Loading

mpkorstanje Jun 2, 2018 •

edited

Loading

mpkorstanje commented Jun 3, 2018 •

edited

Loading

Heziode commented Jun 4, 2018 •

edited

Loading

mpkorstanje commented Jun 4, 2018 •

edited

Loading