onsi / ginkgo

A Modern Testing Framework for Go
http://onsi.github.io/ginkgo/
MIT License
8.22k stars 650 forks source link

Error occurs when trying to avoid spec pollution #1268

Closed iholder101 closed 1 year ago

iholder101 commented 1 year ago

In my test, I've tried to follow the Avoid Spec Pollution: Don't Initialize Variables in Container Nodes rule.

Therefore, I've replaced this code:

var _ = Describe("My test", func() {
    f := framework.NewDefaultFramework("...")
    f.NamespacePodSecurityEnforceLevel = admissionapi.LevelBaseline

With:

var _ = Describe("My test", func() {
    var f *framework.Framework

    BeforeEach(func() {
        f = framework.NewDefaultFramework("...")
        f.NamespacePodSecurityEnforceLevel = admissionapi.LevelBaseline
    })

But after doing so, I'm getting the following error:

  Ginkgo detected an issue with your spec structure
  set up framework | framework.go:200
    It looks like you are trying to add a [BeforeEach] node
    to the Ginkgo spec tree in a leaf node after the specs started running.

    To enable randomization and parallelization Ginkgo requires the spec tree
    to be fully constructed up front.  In practice, this means that you can
    only create nodes like [BeforeEach] at the top-level or within the
    body of a Describe, Context, or When.

    Learn more at:
    http://onsi.github.io/ginkgo/#mental-model-how-ginkgo-traverses-the-spec-hierarchy

I'm not sure what the problem is, and the error description doesn't make a lot of sense for me.

Under this Describe I have a couple of Contexts, containing BeforeEach themselves, and Its. I'd be happy to provide more information if necessary.

Thanks in advance.

iholder101 commented 1 year ago

FWIW, I see that in a LOT of tests (if not all) in the Kubernetes repository the initialization is not made within a BeforeEach block:

kubernetes]> grep "f := framework.NewDefaultFramework(" . -R -B1
/test/e2e/apimachinery/apiserver_identity.go-82-var _ = SIGDescribe("kube-apiserver identity [Feature:APIServerIdentity]", func() {
./test/e2e/apimachinery/apiserver_identity.go:83:       f := framework.NewDefaultFramework("kube-apiserver-identity")
--
./test/e2e/apimachinery/apply.go-47-var _ = SIGDescribe("ServerSideApply", func() {
./test/e2e/apimachinery/apply.go:48:    f := framework.NewDefaultFramework("apply")
--
./test/e2e/apimachinery/chunking.go-47-var _ = SIGDescribe("Servers with support for API chunking", func() {
./test/e2e/apimachinery/chunking.go:48: f := framework.NewDefaultFramework("chunking")
--
./test/e2e/apimachinery/crd_publish_openapi.go-54-var _ = SIGDescribe("CustomResourcePublishOpenAPI [Privileged:ClusterAdmin]", func() {
./test/e2e/apimachinery/crd_publish_openapi.go:55:      f := framework.NewDefaultFramework("crd-publish-openapi")
--
./test/e2e/apimachinery/crd_validation_rules.go-38-var _ = SIGDescribe("CustomResourceValidationRules [Privileged:ClusterAdmin]", func() {
./test/e2e/apimachinery/crd_validation_rules.go:39:     f := framework.NewDefaultFramework("crd-validation-expressions")
--

Am I missing something?

onsi commented 1 year ago

hey @iholder101 - my understanding is that the k8s framework package is designed to augment Ginkgo by wrapping around several Ginkgo constructs to present a consistent interface for the k8s test suite. It's designed to build and configure Ginkgo nodes and is aware of the subtleties around test pollution/variable reuse and is designed to be built and invoked during the Tree Construction phase.

All to say: don't worry about using framework the way most k8s suites use it. That's how its designed to be used. The test pollution guidance applies to variables that are reused and potentially mutated within tests. framework is used to help construct tests.

If you'd like to learn more I'd encourage you to ask questions over at #sig-testing in the k8s slack channel.

iholder101 commented 1 year ago

Thanks a lot @onsi! Appreciate it!

iholder101 commented 1 year ago

However, I would say that maybe the error could be improved in that scenario