Summary with Concurrency has race condition

[smikulcik]

The summary output has incorrect Scenario Step, and Line number references when running with concurrency.

Actual Output

go test -v . --godog.format=progress --godog.concurrency=2 ./features/
........F-......F-F- 20

--- Failed steps:

  Scenario: RUN2 I do a bunch of pass but fail # features/run1.feature:10
    Then I fail RUN1 # features/run1.feature:7
      Error: I fail

  Scenario: RUN1 I do a bunch of pass but fail # features/run1.feature:16
    Then I fail RUN2 # features/run1.feature:13
      Error: I fail

  Scenario: RUN1 I do a bunch of pass but fail # features/run1.feature:16
    Then I fail RUN1 # features/run1.feature:19
      Error: I fail

5 scenarios (2 passed, 3 failed)
20 steps (14 passed, 3 failed, 3 skipped)
364.968µs
testing: warning: no tests to run
PASS
FAIL    godogsummaryfail        0.045s

Expected Output

go test -v . --godog.format=progress --godog.concurrency=2 ./features/
........F-......F-F- 20

--- Failed steps:

  Scenario: RUN1 I do a bunch of pass but fail # features/run1.feature:4
    Then I fail RUN1 # features/run1.feature:7
      Error: I fail

  Scenario: RUN2 I do a bunch of pass but fail # features/run2.feature:10
    Then I fail RUN2 # features/run1.feature:13
      Error: I fail

  Scenario: RUN1 I do a bunch of pass but fail # features/run1.feature:16
    Then I fail RUN1 # features/run1.feature:19
      Error: I fail

5 scenarios (2 passed, 3 failed)
20 steps (14 passed, 3 failed, 3 skipped)
364.968µs
testing: warning: no tests to run
PASS
FAIL    godogsummaryfail        0.045s

Setup

using github.com/DATA-DOG/godog 0.7.10

steps_test.go

// file: steps_test.go
package main

import (
    "flag"
    "fmt"
    "os"
    "testing"

    "github.com/DATA-DOG/godog"
    "github.com/DATA-DOG/godog/colors"
)

var opt = godog.Options{Output: colors.Colored(os.Stdout)}

func init() {
    godog.BindFlags("godog.", flag.CommandLine, &opt)
}

func TestMain(m *testing.M) {
    flag.Parse()
    opt.Paths = flag.Args()

    status := godog.RunWithOptions("godogs", func(s *godog.Suite) {
        FeatureContext(s)
    }, opt)

    if st := m.Run(); st > status {
        status = st
    }
    os.Exit(status)
}

func iPass() error {
    return nil
}

func iFail() error {
    return fmt.Errorf("I fail")
}

func FeatureContext(s *godog.Suite) {
    s.Step(`^I pass`, iPass)
    s.Step(`^I fail`, iFail)
}

features/run1.feature

Feature: RUN1 I try to pass

  Scenario: RUN1 I do a bunch of pass but fail
    Given I pass RUN1
    When I pass RUN1
    Then I fail RUN1
    And I pass RUN1

  Scenario: RUN1 I do a bunch of pass
    Given I pass RUN1
    When I pass RUN1
    Then I pass RUN1
    And I pass RUN1

  Scenario: RUN1 I do a bunch of pass but fail
    Given I pass RUN1
    When I pass RUN1
    Then I fail RUN1
    And I pass RUN1

features/run2.feature

Feature: RUN2 I try to pass

  Scenario: RUN2 I do a bunch of pass
    Given I pass RUN2
    When I pass RUN2
    Then I pass RUN2
    And I pass RUN2

  Scenario: RUN2 I do a bunch of pass but fail
    Given I pass RUN2
    When I pass RUN2
    Then I fail RUN2
    And I pass RUN2

It seems that there is some concurrency bug or race condition that yields inconsistent behavior with the Summary

[smikulcik]

Digging in a little more, it seems that the basefmt in progress mode has a goroutine-local owner property that is not getting set right... I'm not sure why the formatter is the one to maintain which part of the feature file the runner is on...

[l3pp4rd]

thanks for reporting, this needs some refactoring

[l3pp4rd]

the correct solution would be to prevent having this owner node and instead link all gherkin nodes with parent node. this would simplify everything. currently godog uses an older cucumber gherkin version, which does not use protobuf (I do not want to introduce such dependency in this library) most likely I will need to update the current godog version of gherkin.

[ericmcbride]

I wrote a concurrent formatter, and run it in conjunction with the progress formatter. I see the same bug above and was going to open up an issue. I was going to suggest having a pointer to the parent/predecessor node, similar to other AST's i've seen. Not sure if this is the correct way to go about fixing the Node Method. I was able to hack around and make my formatter work for my use case, but would've benefited from the pointer to the parent node in the Node Data structure.

[l3pp4rd]

yes @ericmcbride it would be much better to have a predecessor link in all the nodes. though, I sadly do not have time to work on my OS projects, too busy with family and work. Will have to delay the implementation

[ericmcbride]

I wouldn't mind taking a crack at it @l3pp4rd.

[l3pp4rd]

you are welcome @ericmcbride in case if you see it is too much trouble, I may merge the https://github.com/DATA-DOG/godog/pull/191 which is not a very clean solution and would impact other formatters if we do support concurrency for them later

[ericmcbride]

Im not sure how difficult this would be yet. A co-worker and I have got pretty intimate with the AST and how it works right now, because we have a specialized use case for our concurrent formatter. Do we want to open up a new Issue, since it is a blurry line if the fix is in scope with this issue?

[lonnblad]

I believe this has been solved, I ran the code in the issue and it behaves correctly with the latest version of godog.