Convey should have different traversal modes for stateful v. non-stateful testing

[yuriy-yarosh]

So, it's related to the Issue https://github.com/smartystreets/goconvey/issues/288

In my case it's a side effect when several Conveys with the same name are registered inside a loop:

Convey("SDP Session description should be valid", func() {
    for _, packet := range sdpFixtures {
        for lineNum, line := range strings.Split(packet, "\n") {
            Convey("This one will cause panics", func() {})

            for isVal, typeOrValue := range strings.Split(line, "=") {
                So(typeOrValue, ShouldNotBeNil)
                So(typeOrValue, ShouldNotBeBlank)

                if lineNum == 0 && isVal == 0 {
                    Convey("This one will be executed, if your PC is fast enough to not cause a panic", func() {
                        So(typeOrValue, ShouldEqual, "v")
                    })
                }
            }
        }
    }
})

It's related to the Convey registration inside the convey/context.go:

func (ctx *context) Convey(items ...interface{}) {
    entry := discover(items)

    // we're a branch, or leaf (on the wind)
    if entry.Test != nil {
        conveyPanic(extraGoTest)
    }
    if ctx.focus && !entry.Focus {
        return
    }

    var inner_ctx *context
    if ctx.executedOnce {
        var ok bool
        inner_ctx, ok = ctx.children[entry.Situation]
        if !ok {
            conveyPanic(differentConveySituations, entry.Situation)
        }
    } else {
        if _, ok := ctx.children[entry.Situation]; ok { // LINE 146 
            // I don't think that it's a good idea to panic here, It's better to wait 'till the other convey
            // finished it's execution with a timeout, or just create new one with a suffix
            // like 1, 2, 3, and so on ...
            // or put 'em all inside some kind of container with barrier synchronization etc 
            conveyPanic(multipleIdenticalConvey, entry.Situation)
        }
        inner_ctx = &context{
            reporter: ctx.reporter,

            children: make(map[string]*context),

            expectChildRun: ctx.expectChildRun,

            focus:       entry.Focus,
            failureMode: ctx.failureMode.combine(entry.FailMode),
        }
        ctx.children[entry.Situation] = inner_ctx
    }

    if inner_ctx.shouldVisit() {
        ctxMgr.SetValues(gls.Values{nodeKey: inner_ctx}, func() {
            inner_ctx.conveyInner(entry.Situation, entry.Func)
        })
    }
}

Panics looks like:

panic: Multiple convey suites with identical names: "SDP Session description consists of a number of lines of text of the form: <type>=<value>" [recovered]
    panic: Multiple convey suites with identical names: "SDP Session description consists of a number of lines of text of the form: <type>=<value>" [recovered]
    panic: Multiple convey suites with identical names: "SDP Session description consists of a number of lines of text of the form: <type>=<value>" [recovered]
    panic: Multiple convey suites with identical names: "SDP Session description consists of a number of lines of text of the form: <type>=<value>"

Well, few years ago goconvey has no such side effects, as far as I remember :|

It would be a nice thing to provide an additional ExecutionMode for TDD. So, the test cases would be executed in linear order without additional repeats. It's probably the most common misconception for the newcomers, and the most common reason to switch to the tools like goblin

[riannucci]

adjusting the code to make this panic situation look better may actually be pretty hard without some restructuring; in any event it's an unrecoverable error :/. Convey uses the nested convey names as 'identifiers' and relies on them being unique within a given Convey block.

[yuriy-yarosh]

@riannucci are there any way to make test case execution linear ? It's most obvious and expected behavior.

[riannucci]

No there is not. The way that Convey works is if you do

Convey("A", func() {
  Print("a->b")
  Convey("B", func() {
    Print("b")
  })
  Print("b->c")
  Convey("C", func() {
    Print("c")
  })
  Print("post_c")
})
Print("post_a")

Then the order of execution will be (essentially)

a->b
b
b->c
post_c

a->b
b->c
c
post_c

post_a

The reason for this is that intermediate code between Convey suites is required to prepare preconditions for inner Convey suites; it can't be skipped, and it must be rerun for every leaf test. When you have a loop, Convey needs to start from the top for each leaf Convey suite, and as such, it must have a way to keep track of 'where it needs to go' on every time. You might think that simply using an index of e.g. 'go to the Nth Convey this time' would work, but it breaks down for maps, which have no defined iteration order (e.g. if you iterate over a map for your test cases).

On the flip side, debugging failing tests when all of the suite names are the same is also really confusing; you end up needing to count (or for a map, you would need to guess) which one is having the issue.

FWIW, I usually structure my looped tests like:

var tcs := map[string]struct{
  input int
  output int
}{
  "test name": {1, 3},
}

Convey("Test a function", t, func() {
  for name, tc := range tcs {
    Convey(name, func() {
      So(MyFunc(tc.input), ShouldEqual, tc.output)
    })
  }
})

Sometimes the inputs are all unique, and then I just use fmt.Sprintf to format the name field in the inner convey statement and use a list of testcases, e.g. fmt.Sprintf("MyFunc(%d) -> %d", tc.input, tc.output). That also means that when running the tests in verbose mode, I can see precisely which test case had the failure in it too.

Hopefully that clarifies things?

edit: go syntax

[riannucci]

Oh, and the uniqueness requirement is only within a given suite, so:

Convey("A", func() {
  Convey("Thing", func() {})
})
Convey("B", func() {
  Convey("Thing", func() {})
})

Is perfectly fine. This means that if you have multiple Convey suites per looped test case, you only need to differentiate them at the top of the loop: all the 'duplicate' suites inside of that disambiguated header can have the same names:

var tcs := []struct{input string, ...}{...}

Convey("MyFunc", func() {
  for _, tc := range tcs {
    Convey(fmt.Sprintf("with input %d", tc.input), func() {
      Convey("Should do blah", func() {...})
      Convey("Should do bleet", func() {...})
    }) 
  }
})

[yuriy-yarosh]

@riannucci I do know how Convey works, thanks dude. Just don't get why it's so important to "prepare preconditions for inner Convey suites". I think that a lot of people don't need to rerun their cases from scratch for every leaf and branch of the dependency tree - it's very naive and ineffective approach. I'll keep on rerunning same test cases hundreds of times over and over, just because they are too nested and inside loops.

Well, It's the reason why I'm ditching goconvey now. It's great tool, but I don't like it's internal and strict design. it's a bit dumb that I can't customize execution order for my own purposes, and perform proper resource management, even if it means adding few (5-6) keywords and few (2-3) interfaces.

[riannucci]

I think it's perfectly fine to have different tools for different purposes. I thought this issue was a question about why it works the way it does, not an RFC for a new feature.

Do you have a proposed design that would make Convey work as-is for spec based tests, but also have some extension that works better for tdd tests?

The reason that precondition re-execution is important for many folks is that very frequently this sort of thing happens:

Convey("Test something", func() {
  db := SetupDB(...)

  for _, tc := range tcs {
    Convey(tc.name, func() {
      db.Insert(tc.dbPrecondition)

      Convey("Thing 1", func() {
        So(MyFunc(db, tc.input), ShouldEqual, tc.output)

        Convey("Thing 2 (assuming thing 1)", func() {
          So(MyOtherFunc(db, tc.input2), ShouldEqual, tc.output2)
        })
      })
    })
  }  
})

If you just ran all the tests in order, it means that you'd be re-using the db from the previous loop, which can lead to false coupling between tests. Yet, the db.Insert precondition and the call-function2-with-the-effects-of-function1 are both desired accumulations of state. Yes, you could isolate these tests to make them each prepare their own preconditions redundantly, but the way the code would work in production (e.g. accumulating state as the result of user interactions) is closer to what's written above, and many folks prefer to have their test flows match the production flows. Capturing the behavior of MyOtherFunc after MyFunc is more important to test in this example than testing that MyOtherFunc behaves after what it thinks is the output from MyFunc. If the behavior of MyFunc changes and no longer sets the correct prerequisites for MyOtherFunc, this test would catch it, but independent tests might not.

AFAICT, goblin solves this problem in the more traditional way, by explicitly having setup/teardown callbacks for each suite of tests. That's not really the way that convey is designed; all setup (code between Convey's and So's) and teardown (deferred functions) just happen by virtue of being present in the code, no special syntax necessary. There's nothing wrong with that, it's just two different designs to solve the same problem.

[yuriy-yarosh]

Yeah, @riannucci I get it... I'm talking about TDD like style spec testing, and Goconvey was primarily designed for BDD, I get it too. I do understand that you're saying that if fixtures got any shared state we have to reset it manually for every test case, and Goconvey was designed to use a single Convey command for that in mind.

I can't propose any designs without traditional setup / teardown logic, but it's a viable option only for fixtures that shares a state and could be treated as FSMs, i.e. Databases. File based fixtures and fuzzers don't have any internal shared state, so it's obviously better to stick with linear execution there.

I'll probably fork goconvey and implement shared resources management mechanism, maybe create a specific SQL database connection wrapper etc. So, goconvey would be able to decide between tree-like execution and plain old obvious linear one.

[riannucci]

Maybe if we added a function you could put in a Convey suite like ISolemnlySwearThereAreNoSideEffects() or something. Then Convey could use that to continue executing instead of resetting. Haven't thought it through entirely, but I think the primary issue here is that Convey assumes that there are side effects for every Convey suite. Not sure what the precise syntax would be. Could be a different Convey function, or just dropping a new function call inside the suite, or something like that?

[riannucci]

(and maybe it could be a sticky mode change, like:

Convey("Test", t, func() {
  AssumeSideEffects(false)
  ... everything under here assumes no side effects ...
  Convey("Something in here", func() {
    ... still assuming no side effects ...
    Convey("Something else", func() {
      AssumeSideEffects(true)
      ... Now Convey will reset back to this suite for every leaf found under here ... 
    })
  })
})

Probably needs to be a different syntax though; this syntax would encourage folks to think they can switch it back and forth inside of a given suite, which would be confusing at best... it's really a property of the suite. Maybe ConveyWithoutSideEffects ?

[yuriy-yarosh]

@riannucci, I think CAS-like stuff might be sufficient enough. You can pass a set of pointers of the shared variables and the database connection directly into the Convey(), than after test case had been finished, you can check if any internal state of the DB or passed variables had been actually changed, and make a decision how to proceed with test case execution order: if state actually had been changed - use tree based one, if state hadn't been changed - use linear based one, or at least something like that. DB connection would probably require a wrapper with a set of hooks, but it's not a hard thing to do. ConveyWithoutSideEffects sounds a bit like a joke. I can't imagine any other way that would be able to save backwards compatibility with existing API, without breaking anything or adding any new constructs.

[riannucci]

Could you show how it would look? I honestly wasn't trying to make a joke... The execution order thing has bothered me in the past but I never had a good solution for it that didn't just turn convey into some other testing library. Maintaining backwards compatibility is pretty high priority at this point.

On Sun, Apr 24, 2016, 12:45 Юрий Ярош notifications@github.com wrote:

@riannucci https://github.com/riannucci, I think CAS-like stuff might be sufficient enough. You can pass a set of pointers of the shared variables and the database connection directly into the Convey(), than after test case had been finished, you can check if any internal state of the DB or passed variables had been actually changed, and make a decision how to proceed with test case execution order: if state actually had been changed - use tree based one, if state hadn't been changed - use linear based one, or at least something like that. DB connection would probably require a wrapper with a set of hooks, but it's not a hard thing to do. ConveyWithoutSideEffects sounds a bit like a joke.

— You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub https://github.com/smartystreets/goconvey/issues/425#issuecomment-214027727

[yuriy-yarosh]

@riannucci sure thing bro, sure thing.

So my point is that we're getting side effects while mutating existing resource's states, so if we're checking if the actuall resource had been actually mutated we can decide what should be rerun and what should not.

Convey("A", t, func() {
  db := SetupDB(...)
  Convey("B", func() {
    // We are Inserting / Updating DB records here
    oneVar := "one"
    db.Insert()
    db.Update()
    Convey("1", func() {}).With(&oneVar)

    Convey("2", func() {
      oneVar = "newValue"
      db.Insert()
      db.Update()

      Convey("X",func() {
        db.Insert()
        db.Update()
      }).WithDb(&db).With(&oneVar)

      Convey("Y",func() {
      }).WithDb(&db).With(&oneVar)
    }).WithDb(&db).With(&oneVar)

    Convey("3", func() {
    }).With(&oneVar)

  }).WithDb(&db)

 Convey("C", func() {}).WithDb(&db)
})

A B 1 2X A B 2Y A B 3 A C

So the value that is passed into the Convey from the upper context is treated like the immutable one. And obviously we're not getting any side effects here.

It's probably a good idea to write a linter for that, so people would not be able to access upper context variables which hadn't been .With()ed for the respective Convey() call. It's wise to run such linter before any actual testing.

So, what do you think ?

[riannucci]

Hm.. That sounds kinda unsafe to me... If you have some stack object which is mutable (like a struct, or a struct with a pointer to something else), you'd have to do some 'deep equals' comparison on it to tell, and the user would need to remember to put With statements around all the pertinent variables (which makes test cases much harder to write and maintain). It also changes the default assumption of goconvey from has-side-effects to doesn't-have-side-effects, which means that all existing tests would run incorrectly after the change :/ .

Having a linter sounds like a hack to me (but I agree, this would be unsafe without such a linter). Though, if you had a linter that was accurate, why couldn't it just figure out the Withs itself (e.g. like some sort of go-generate program)?

Since the default assumption in convey is that suites do have stateful effects, I don't think this would work... any proposal here needs to be as an optional optimization over the existing behavior; current tests should continue to work as they always have, but new tests could take advantage of some new syntax to achieve better execution characteristics.

[yuriy-yarosh]

Yeah, you're right, but deep equal is not a must here - if were going to drop With statements, parse existing test cases, and take advantage of the go generate to add something like Mutating() call. Mutating() construct might be added by hand too for some HTTP and other stateful testing. Of course you can name it with a better name :3

So, it would look like this

Convey("A", t, LinearExecution, func() {
  db := SetupDB(...)

  Convey("B", func() {
    oneVar := "one"
    db.Insert()
    db.Update()
    Mutating()
    Convey("1", func() {})

    Convey("2", func() {
      oneVar = "newValue"
      Mutating()

      db.Insert()
      db.Update()
      Mutating()
      Convey("X",func() {
        db.Insert()
        db.Update()
        Mutating()
      })

      Convey("Y",func() {
      })
    })

    Convey("3", func() {
    })
  })

 Convey("C", func() {})
})

[riannucci]

(reopening this so we don't lose track of it)

I think this Mutating proposal is pretty similar to what I was suggesting with the side effects function. I'm not sure if it can be as granular as you propose.

[riannucci]

(I'm not sure what the right way to implement this would be, but I think this discussion is valuable enough to not be hidden in a closed issue)