Add --retry option to retry failed tests as part of the same run #920

danascheider · 2015-10-03T06:28:57Z

Here are the features I wrote up for #882. Let me know what you think, @mattwynne and @brasmusson. Travis isn't passing but it's those wire protocol features again - shouldn't have been affected by what I wrote.

mattwynne · 2015-10-06T15:48:05Z

Looks like a good start but there's no implementation right? So the tests should not be passing... or did I misunderstand something?

mattwynne · 2015-10-06T15:48:51Z

You could usefully rebase your branch (you can use git rebase -i) then push --force to clean up the commit history on this PR - it makes it easier to read.

danascheider · 2015-10-06T18:54:09Z

I think I was the one who misunderstood something. A while ago you said something about failing features being the first step, and I assumed that meant you wanted a PR with the tests. I’m giving a 45-minute talk tonight so things have been kind of crazy, but I’ll have a chance to work on this more later this week. And re: git rebase -i - I meant to do that before, sorry!

On Oct 6, 2015, at 8:48 AM, Matt Wynne [email protected] wrote:

Looks like a good start but there's no implementation right? So the tests should not be passing... or did I misunderstand something?

—
Reply to this email directly or view it on GitHub #920 (comment).

mattwynne · 2015-10-13T18:11:27Z

Comprende! I was confused because the branch was green, but that's because you've changed the travis config is it?

So yeah I think those scenarios are a good place to start. We need to update the description a bit to align with what we've decided (i.e. deferring things like using tags).

mattwynne · 2015-10-13T18:12:09Z

You happy to have a crack at an implementation Dana? Give me a shout if you need some support. Thanks for taking interest in this!

brasmusson · 2015-10-13T18:20:34Z

@mattwynne retry_failing_tests.feature is tagged @wip, therefore build is green.

danascheider · 2015-10-13T19:46:34Z

I'm totally working on it already, just didn't want to pollute the existing PR with all the new commits so I switched to a different branch & will merge later.

danascheider · 2015-10-13T19:59:09Z

Happy to help!

mattwynne · 2015-10-15T06:39:45Z

I'm totally working on it already, just didn't want to pollute the existing PR with all the new commits so I switched to a different branch & will merge later.

OK. I've put the WIP tag on this PR, so it would work well for me if you just use this branch as you work on the solution, rebase and push --force as much as you like and just keep sharing the latest with us. That way I can stay involved and help. That's if you want any help of course!

danascheider · 2015-10-15T07:15:54Z

That works for me. I should have some time to work on this tomorrow as I just finished a couple other projects that were taking up my time.

On Oct 14, 2015, at 11:39 PM, Matt Wynne [email protected] wrote:

I'm totally working on it already, just didn't want to pollute the existing PR with all the new commits so I switched to a different branch & will merge later.

OK. I've put the WIP tag on this PR, so it would work well for me if you just use this branch as you work on the solution, rebase and push --force as much as you like and just keep sharing the latest with us. That way I can stay involved and help. That's if you want any help of course!

—
Reply to this email directly or view it on GitHub.

danascheider · 2015-10-15T21:45:59Z

OK, I didn't end up getting to this today because my server got rooted and I had to fix everything. Keeping my fingers crossed for tomorrow!

mattwynne · 2015-10-15T22:15:41Z

Friday is OSS day so that's good. Grab me in https://gitter.im/cucumber/cucumber-ruby if you want to chat.

mattwynne · 2015-10-22T21:03:47Z

lib/cucumber/filters/retry.rb

+      def test_case(test_case)
+        super 
+
+        configuration.on_event(:after_test_case) do |event|


I'd have thought you'd need to register the event listener earlier than this, like maybe in the filter's constructor. Does it work?

danascheider · 2016-01-11T19:02:51Z

@mattwynne I got a little stuck trying to figure out why the one spec on this still fails. It seems to be saying that when there's a failing test (not a flaky one), it's only retrying once, even though the configuration options say it should be retried twice. Am I missing something obvious?

mattwynne · 2016-01-20T21:17:46Z

I have a flight on Monday when I should be able to give this some focus. Can you push up whatever you have (use another branch if you want) and I'll get my head around it then?

Thanks for your patience @danascheider.

danascheider · 2016-01-23T19:47:05Z

@mattwynne No problem, I'm pretty sure what's on this branch is all I have at this point, but I'll check my home laptop for more and push anything I find. I've been really busy too, so no worries.

mattwynne · 2016-03-20T11:02:10Z

@danascheider FINALLY I got a chance to look at this 😀. I'm lucky enough to have @brasmusson in the same room as me today, and we figured out some subtle problems that were preventing the retry filter from working quite right. We pushed them here: 8f9445f for you to take a look at. I wouldn't copy the changes line-for-line as we were messing around quite a bit and probably changed more than is necessary. I'll put some comments on that diff to show you the interesting bits.

FYI, the acceptance test now seems to be failing for another reason - even when a test has been retried, the summary still shows 1 scenario. @brasmusson and I are looking into that now.

danascheider · 2016-03-20T17:01:35Z

@mattwynne Thank you! I'll take a look at it too.

mattwynne · 2016-04-10T18:20:17Z

Hi @danascheider, just checking in to see how you're feeling about this one. I'm deliberately leaving it so you can get the sense of satisfaction from finishing it. Is there anything blocking you right now that I can help with?

danascheider · 2016-04-10T20:44:15Z

Oh, thanks, @mattwynne. It's on my list, I'm just working a lot and in the middle of moving so I had it on the back burner.

mattwynne · 2016-04-11T12:42:29Z

👍🚚💼

Take your time @danascheider, I think you have most of the puzzle pieces now. The existing code for the summary numbers in the formatters is a bit of a muddle but that's a problem we can deal with when you've got the main behaviour working.

Redo the commit I just reverted but with the file I intended to include the first time

Change Retry filter to have attribute :configuration and first pass at setting up a listener Reorganize spec file Finish adding tests for re-running flaky test cases - all but one pass Fix spec that said to test what happens when a test case passes but actually didn't make sure it did pass

Define steps for the retry feature

danascheider · 2016-05-15T03:17:35Z

Got all the tests passing - any other thoughts, @mattwynne or @brasmusson? What was the problem with the formatter for the summary numbers?

brasmusson · 2016-05-15T19:35:51Z

An reasonable output from the progress formatter in case of one scenario that passed, one scenario that passed after one retry, one scenario that passed after two retries, and one scenario that still failed after two retries could be:

..R.RR.RRF
<exception stacktraces>
4 Scenarios (3 Passed, 1 Failed) - 2 Scenarios needed retires to pass

A problem with the implementation is that the summary is provided by the Console module, it is used both by the pretty formatter and the progress formatter (and peoples custom formatter), and the progress formatter is on the way to be fully based on events (#977) using the Summary class from the Core , whereas the pretty formatter is still using the legacy formatter API. To implement what is needed to produce a summary as the one above which works for all the APIs that formatters uses seems to be - how should I put it - not trivial.

mattwynne · 2016-05-15T21:58:18Z

To implement what is needed to produce a summary as the one above which works for all the APIs that formatters uses seems to be - how should I put it - not trivial.

Classic Swedish understatement @brasmusson!

Actually I don't think it will be that hard, but equally I think it's an enhancement. With what @danascheider's already done, the thing is usable and useful. We can improve on the reporting in another iteration IMO.

Thoughts?

mattwynne · 2016-05-15T22:00:07Z

features/lib/step_definitions/cucumber_steps.rb

@@ -32,6 +32,70 @@
  create_step_definition { string }
 end

+Given /^a scenario "([^\"]*)" that fails once, then passes$/ do |name|


Do you think maybe these should go into a retry_steps.rb? They're quite specific...

mattwynne · 2016-05-15T22:07:28Z

There's lot of commits here. Probably best to squash when this gets merged? Unless you want to rebase @danascheider?

danascheider · 2016-05-15T22:12:49Z

Sure, they can be squashed.

brasmusson · 2016-05-26T19:08:15Z

lib/cucumber/filters/retry.rb

+      private
+
+      def retry_required?(test_case, event)
+        event.test_case == test_case && event.result.failed? && test_case_counts[test_case] < configuration.retry_attempts


This does not work, since the test_case you pass on is never the same that are in the after_test_case event. Filters like activate_steps call Case#with_steps which creates a new test case object.

I've resolved this with cucumber/cucumber-ruby-core#111 which makes equality for Test::Case objects work as you would expect it to.

@mattwynne

…umber#920) * Add feature file from PR 895 to start work on retry functionality * Initial spec for --retry option * Add another test for --retry flag, make new tests pass * Squash some commits Redo the commit I just reverted but with the file I intended to include the first time * Rebase onto master * Allow for --retry option in configuration Add retry_attempts method to configuration base class Add Retry formatter and specs for same Remove files attempting to implement --retry with a formatter - bad idea Add wip tag to prevent --retry features from killing the build Actually run Travis build against active branch Fix tiny grammatical error. Closes cucumber#914. * Update scenarios to look like @mattwynne's suggestion * Initial specs for retry filter - they test if it works at all, basically Change Retry filter to have attribute :configuration and first pass at setting up a listener Reorganize spec file Finish adding tests for re-running flaky test cases - all but one pass Fix spec that said to test what happens when a test case passes but actually didn't make sure it did pass * Add consistently failing scenario to test cases Define steps for the retry feature * Correct # of retries * Refactor test case * Make method private * Extract retry_required? method * Move steps for retry filter into other file * Correct block style

lock · 2018-10-25T05:29:11Z

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.

mattwynne added the WIP label Oct 15, 2015

danascheider force-pushed the resolve-issue-882 branch from 43dac6a to 7a25d81 Compare October 19, 2015 18:37

mattwynne reviewed Oct 22, 2015
View reviewed changes

mattwynne changed the title ~~Features for --retry~~ Add --retry option to retry failed tests as part of the same run Apr 10, 2016

mattwynne mentioned this pull request Apr 19, 2016

add an initial feature for rerunning failing tests #895

Closed

danascheider added 4 commits May 12, 2016 13:14

Add feature file from PR 895 to start work on retry functionality

745fb95

Initial spec for --retry option

ba28a95

Add another test for --retry flag, make new tests pass

28bf14e

Squash some commits

788a03e

Redo the commit I just reverted but with the file I intended to include the first time

danascheider added 3 commits May 12, 2016 13:15

Update scenarios to look like @mattwynne's suggestion

5ca0ce8

Add consistently failing scenario to test cases

d5979ad

Define steps for the retry feature

danascheider force-pushed the resolve-issue-882 branch from 7a25d81 to d5979ad Compare May 12, 2016 20:15

danascheider added 4 commits May 14, 2016 20:06

Correct # of retries

3064b27

Refactor test case

b05303f

Make method private

130a686

Extract retry_required? method

3388fe2

danascheider removed the WIP label May 15, 2016

mattwynne reviewed May 15, 2016
View reviewed changes

danascheider added 2 commits May 15, 2016 15:11

Move steps for retry filter into other file

7fddbed

Correct block style

7f4b9f2

mattwynne merged commit 5cae2b2 into cucumber:master May 17, 2016

mattwynne added a commit that referenced this pull request May 17, 2016

Update history with #920

ccbe58b

mattwynne mentioned this pull request May 20, 2016

Remove tolerance for @wip scenarios in build #981

Closed

3 tasks

brasmusson mentioned this pull request May 22, 2016

Integrate new core events #973

Merged

3 tasks

danascheider mentioned this pull request May 26, 2016

Remove wip tag #982

Closed

brasmusson reviewed May 26, 2016
View reviewed changes

richarda pushed a commit to richarda/cucumber that referenced this pull request Jul 1, 2016

Update history with cucumber#920

cf1c96b

mattwynne mentioned this pull request Jul 2, 2016

Polish off retry #992

Merged

2 tasks

brasmusson added a commit that referenced this pull request Jul 3, 2016

Put back notifications removed in #920.

ed2e77e

lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --retry option to retry failed tests as part of the same run #920

Add --retry option to retry failed tests as part of the same run #920

danascheider commented Oct 3, 2015

mattwynne commented Oct 6, 2015

mattwynne commented Oct 6, 2015

danascheider commented Oct 6, 2015

mattwynne commented Oct 13, 2015

mattwynne commented Oct 13, 2015

brasmusson commented Oct 13, 2015

danascheider commented Oct 13, 2015

danascheider commented Oct 13, 2015

mattwynne commented Oct 15, 2015

danascheider commented Oct 15, 2015

danascheider commented Oct 15, 2015

mattwynne commented Oct 15, 2015

mattwynne Oct 22, 2015

danascheider commented Jan 11, 2016

mattwynne commented Jan 20, 2016

danascheider commented Jan 23, 2016

mattwynne commented Mar 20, 2016

danascheider commented Mar 20, 2016

mattwynne commented Apr 10, 2016

danascheider commented Apr 10, 2016

mattwynne commented Apr 11, 2016

danascheider commented May 15, 2016

brasmusson commented May 15, 2016

mattwynne commented May 15, 2016

mattwynne May 15, 2016

danascheider May 15, 2016

mattwynne commented May 15, 2016

danascheider commented May 15, 2016

brasmusson May 26, 2016

mattwynne Jul 2, 2016

lock bot commented Oct 25, 2018

Add --retry option to retry failed tests as part of the same run #920

Add --retry option to retry failed tests as part of the same run #920

Conversation

danascheider commented Oct 3, 2015

mattwynne commented Oct 6, 2015

mattwynne commented Oct 6, 2015

danascheider commented Oct 6, 2015

mattwynne commented Oct 13, 2015

mattwynne commented Oct 13, 2015

brasmusson commented Oct 13, 2015

danascheider commented Oct 13, 2015

danascheider commented Oct 13, 2015

mattwynne commented Oct 15, 2015

danascheider commented Oct 15, 2015

danascheider commented Oct 15, 2015

mattwynne commented Oct 15, 2015

mattwynne Oct 22, 2015

Choose a reason for hiding this comment

danascheider commented Jan 11, 2016

mattwynne commented Jan 20, 2016

danascheider commented Jan 23, 2016

mattwynne commented Mar 20, 2016

danascheider commented Mar 20, 2016

mattwynne commented Apr 10, 2016

danascheider commented Apr 10, 2016

mattwynne commented Apr 11, 2016

danascheider commented May 15, 2016

brasmusson commented May 15, 2016

mattwynne commented May 15, 2016

mattwynne May 15, 2016

Choose a reason for hiding this comment

danascheider May 15, 2016

Choose a reason for hiding this comment

mattwynne commented May 15, 2016

danascheider commented May 15, 2016

brasmusson May 26, 2016

Choose a reason for hiding this comment

mattwynne Jul 2, 2016

Choose a reason for hiding this comment

lock bot commented Oct 25, 2018