Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Standardize gomega/ginkgo imports #2433

Closed
chuckha opened this issue Feb 25, 2020 · 6 comments · Fixed by #2736
Closed

Standardize gomega/ginkgo imports #2433

chuckha opened this issue Feb 25, 2020 · 6 comments · Fixed by #2736
Assignees
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor.
Milestone

Comments

@chuckha
Copy link
Contributor

chuckha commented Feb 25, 2020

This issue is to standardize how to import ginkgo/gomega.

If we are using ginkgo/gomega in a test, it should be imported using the . import like this:

import (
    . "github.com/onsi/ginkgo"
    . "github.com/onsi/gomega"
)

The reason to do this is to provide a consistent feel across all ginkgo/gomega tests in the codebase.

/kind feature
/help
/milestone Next

@detiber
Copy link
Member

detiber commented Feb 25, 2020

There is at least one place where the gomega imports (when using .) conflict with functions defined (controlplane/kubeadm/internal).

I'd argue that we should prefer readability of shipped code paths over the readability of tests, so if we are not willing to make exceptions for tests that would test packages that may have conflicting naming to ginkgo/gomega, then I would suggest that we not use . imports for ginkgo/gomega and instead be explicit in our use of ginkgo/gomega (ginkgo.Eventually, gomega.Expect, etc).

@k8s-ci-robot k8s-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Feb 25, 2020
@k8s-ci-robot k8s-ci-robot added this to the Next milestone Feb 25, 2020
@k8s-ci-robot k8s-ci-robot added the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Feb 25, 2020
@cpanato
Copy link
Member

cpanato commented Mar 2, 2020

let me try this one
/assign

@cpanato
Copy link
Member

cpanato commented Mar 2, 2020

for the existing tests that already have the . should we update to remove that?

@randomvariable
Copy link
Member

probably.

assuming you've started on this, so will mark as
/lifecycle active

@k8s-ci-robot k8s-ci-robot added the lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor. label Mar 2, 2020
@vincepri
Copy link
Member

vincepri commented Mar 2, 2020

for the existing tests that already have the . should we update to remove that?

apart from control plane one, let's try to use the dot import everywhere else and find a different solution there

k8s-ci-robot added a commit that referenced this issue Mar 8, 2020
🏃tests: standardize gomega/ginkgo imports
k8s-ci-robot added a commit that referenced this issue Mar 9, 2020
🏃 tests: standardize gomega imports - Follow up
k8s-ci-robot added a commit that referenced this issue Mar 10, 2020
🏃cmd-clusterctl-api/tests: standardize gomega imports - follow up
k8s-ci-robot added a commit that referenced this issue Mar 11, 2020
🏃cmd-clusterctl-client-config/tests: standardize gomega imports
k8s-ci-robot added a commit that referenced this issue Mar 11, 2020
🏃cmd-clusterctl-client-cluster/tests: standardize gomega imports
k8s-ci-robot added a commit that referenced this issue Mar 11, 2020
🏃 bootstrap/tests: standardize gomega imports - follow up
k8s-ci-robot added a commit that referenced this issue Mar 13, 2020
🏃cmd-clusterctl-client-repository/test: standardize gomega imports
k8s-ci-robot added a commit that referenced this issue Mar 13, 2020
🏃 controllers/test: standardize gomega imports
k8s-ci-robot added a commit that referenced this issue Mar 16, 2020
🏃 cmd-clusterctl-client/tests: standardize gomega imports
k8s-ci-robot added a commit that referenced this issue Mar 16, 2020
🏃 util/tests: standardize gomega imports
k8s-ci-robot added a commit that referenced this issue Mar 16, 2020
🏃test/tests: standardize gomega imports
k8s-ci-robot added a commit that referenced this issue Mar 16, 2020
🏃tests: standardize gomega imports for controllers/mdutil and controllers/noderefutil
@cpanato
Copy link
Member

cpanato commented Mar 17, 2020

/remove-help

@k8s-ci-robot k8s-ci-robot removed the help wanted Denotes an issue that needs help from a contributor. Must meet "help wanted" guidelines. label Mar 17, 2020
@vincepri vincepri modified the milestones: Next, v0.3.x Mar 17, 2020
k8s-ci-robot added a commit that referenced this issue Mar 19, 2020
🏃 controlplane-kubeadm/test: standardize gomega imports
k8s-ci-robot added a commit that referenced this issue Mar 23, 2020
🏃 tests: standardize gomega imports
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature Categorizes issue or PR as related to a new feature. lifecycle/active Indicates that an issue or PR is actively being worked on by a contributor.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants