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

🌱 Refactor tests to plain go in util/collections #4597

Merged

Conversation

mboersma
Copy link
Contributor

What this PR does / why we need it:

Refactors the tests in util/collections/machine_collection_test.go to use plain Go testing instead of ginkgo. The other tests in this package were already there.

Which issue(s) this PR fixes:
Refs #4588

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 11, 2021
@enxebre
Copy link
Member

enxebre commented May 11, 2021

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2021
util/collections/machine_collection_test.go Outdated Show resolved Hide resolved
util/collections/machine_collection_test.go Outdated Show resolved Hide resolved
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only nit I have on this would be that we are losing some of the context on the test names with this change, for example we lost the SortedByAge context on the first test which I think is important to keep. Could you double check and maybe adjust the test names as appropriate?

@mboersma
Copy link
Contributor Author

we are losing some of the context on the test names with this change

Indeed, I'll try to preserve that logging context as well.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 11, 2021
@mboersma
Copy link
Contributor Author

mboersma commented May 11, 2021

Since Describe and friends aren't available to us without ginkgo, the test reporting context looks much different by default.

With ginkgo:

• Failure [0.000 seconds]
Machine Collection
/Users/matt/Projects/src/sigs.k8s.io/cluster-api/util/collections/machine_collection_test.go:36
  Machines
  /Users/matt/Projects/src/sigs.k8s.io/cluster-api/util/collections/machine_collection_test.go:37
    SortedByAge
    /Users/matt/Projects/src/sigs.k8s.io/cluster-api/util/collections/machine_collection_test.go:48
      should return the same number of machines as are in the collection [It]
      /Users/matt/Projects/src/sigs.k8s.io/cluster-api/util/collections/machine_collection_test.go:49

      Expected
          <int>: 1
      to equal
          <int>: 2

      /Users/matt/Projects/src/sigs.k8s.io/cluster-api/util/collections/machine_collection_test.go:53
------------------------------
••

Summarizing 1 Failure:

[Fail] Machine Collection Machines SortedByAge [It] should return the same number of machines as are in the collection 
/Users/matt/Projects/src/sigs.k8s.io/cluster-api/util/collections/machine_collection_test.go:53

Without ginkgo:

--- FAIL: TestMachineCollection (0.00s)
    --- FAIL: TestMachineCollection/should_return_the_same_number_of_machines_as_are_in_the_collection (0.00s)
        machine_collection_test.go:35: 
            Expected
                <int>: 1
            to equal
                <int>: 2
FAIL
FAIL	sigs.k8s.io/cluster-api/util/collections	0.476s

I could annotate the first argument to t.Run() to include this context:

    --- FAIL: TestMachineCollection/Machine_Collection/Machines/SortedByAge/should_return_the_same_number_of_machines_as_are_in_the_collection (0.00s)

or maybe just:

    --- FAIL: TestMachineCollection/SortedByAge/should_return_the_same_number_of_machines_as_are_in_the_collection (0.00s)

@mboersma
Copy link
Contributor Author

BTW, I verified that go test ./util/collections -count 2 fails in master but succeeds on this branch. That supports our assumption in #4183.

@mboersma
Copy link
Contributor Author

@JoelSpeed I nested the t.Run() statements so it looks and acts more like the ginkgo spec it replaces:

--- FAIL: TestMachineCollection (0.00s)
    --- FAIL: TestMachineCollection/SortedByAge (0.00s)
        --- FAIL: TestMachineCollection/SortedByAge/should_return_the_same_number_of_machines_as_are_in_the_collection (0.00s)
            machine_collection_test.go:36: 
                Expected
                    <int>: 1
                to equal
                    <int>: 2
FAIL
FAIL	sigs.k8s.io/cluster-api/util/collections	0.374s

@JoelSpeed
Copy link
Contributor

That works for me, thanks!

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 12, 2021
Copy link
Member

@vincepri vincepri left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve
/lgtm

@vincepri
Copy link
Member

/milestone v0.4

@k8s-ci-robot k8s-ci-robot added this to the v0.4 milestone May 12, 2021
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: vincepri

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 12, 2021
@vincepri
Copy link
Member

Thank you @mboersma !

@k8s-ci-robot k8s-ci-robot merged commit a2b976d into kubernetes-sigs:master May 12, 2021
@mboersma mboersma deleted the go-test-util-collections branch May 12, 2021 15:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants