Remove the BMCClient interface, ginkgo, and gomega #102

jacobweinstock · 2023-05-05T23:06:04Z

Description

Why is this needed

The BMCClient interface was unnecessary and added complexity to testing. bmclib already has the capabilities to specify mock providers. Removing this interface also allowed us to remove ginkgo and gomega. These frameworks were unneeded and overly complex. This PR simplifies tests, increases coverage, and sets us up to build out more tests quickly and simply.

Fixes: #59

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

I have:

updated the documentation and/or roadmap (if required)
added unit or e2e tests
provided instructions on how to upgrade

codecov · 2023-05-05T23:11:19Z

Codecov Report

Merging #102 (ae0ada1) into main (458449e) will increase coverage by 3.71%.
The diff coverage is 57.44%.

❗ Current head ae0ada1 differs from pull request most recent head 40bb987. Consider uploading reports for the commit 40bb987 to get more accurate results

@@            Coverage Diff             @@
##             main     #102      +/-   ##
==========================================
+ Coverage   58.85%   62.57%   +3.71%     
==========================================
  Files           4        4              
  Lines         350      350              
==========================================
+ Hits          206      219      +13     
+ Misses        107       97      -10     
+ Partials       37       34       -3

Impacted Files	Coverage Δ
controllers/client.go	`0.00% <0.00%> (ø)`
controllers/job.go	`55.28% <57.14%> (ø)`
controllers/task.go	`63.93% <70.37%> (ø)`
controllers/machine.go	`80.21% <83.33%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

chrisdoherty4

Some great improvements. Glad to be removing Ginkgo.

chrisdoherty4 · 2023-05-06T15:10:13Z

controllers/task_controller_test.go

-	for name, tt := range map[string]struct {
-		configureClientCalls func(expect *mocks.MockBMCClientMockRecorder)
-		action               v1alpha1.Action
+func TestTaskReconcile(t *testing.T) {


The body of the test func has a lot going on. I'm not sure it helps understandability compared to breaking into a few separate table tests. For example, there seems to be different test branches such as the t.timeoutErr that might help readability if it were a separate top level test.

What's the motivation to bundle all tests into a single table test?

this was mostly a translation of the ginkgo/gomega tests. The idea was to start by removing those libs and then refactoring the tests. I can do the refactor in this PR if you'd like.

chrisdoherty4 · 2023-05-06T21:51:36Z

api/v1alpha1/job.go

 type Job struct {
-	metav1.TypeMeta   `json:",inline"`


I'm weary of removing inline because every object I've ever seen does it. I'm not 100%, but I think its an option used by Kubernetes client-go json/yaml implementation. Its also documented in examples in the kubebuilder book https://book.kubebuilder.io/cronjob-tutorial/new-api.html.

inline is not a recognized struct tag in Go. I believe the only reason to keep them is if we have any code generation that depends on it. I ran make generate and make manifests and don't think I saw any issues. I could be mistaken though.

ref:
golang/go#6213
kubernetes/kubernetes#117206

Thanks, this has plagued me with the linting and I could never find anything useful about it.

chrisdoherty4 · 2023-05-06T21:54:52Z

controllers/job.go

 }

-func (r *JobReconciler) reconcile(ctx context.Context, job *bmcv1alpha1.Job, jobPatch client.Patch, logger logr.Logger) (ctrl.Result, error) {
+func (r *JobReconciler) doReconcile(ctx context.Context, job *v1alpha1.Job, jobPatch client.Patch) (ctrl.Result, error) {


Do we need the do prefix? do prefixes don't really add anything.

this was a linting issue that came up. I'm open to changing this. I can revert back and add a notlint or change the name entirely or see about removing the check from the lint config. Thoughts?

chrisdoherty4 · 2023-05-06T21:56:50Z

controllers/job_test.go

+		secret    *corev1.Secret
+		job       *v1alpha1.Job
+		shouldErr bool
+		testAll   bool


What are the semantics of testAll?

testAll allows this test to do more testing than just if reconciler.Reconcile was successful. This is an artifact of translating the ginkgo/gomega tests. I will refactor, just depends on if you would like that done in this PR or a follow-up. This PR is fairly large as is.

controllers/job_test.go

chrisdoherty4 · 2023-05-10T12:18:49Z

@Mergifyio queue

mergify · 2023-05-10T12:18:52Z

queue

🛑 The pull request has been removed from the queue

Pull request #102 has been dequeued due to failing checks or checks timeout.

You can take a look at Queue: Embarked in merge train check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

This makes it easier to run the linting. And ci will run the exact same linting. Signed-off-by: Jacob Weinstock <[email protected]>

The BMCClient interface was unnecessary and added complexity to testing. bmclib already has the capabilities to specify mock providers. Removing this interface also allowed us to remove ginkgo and gomega. These frameworks were unneeded and overly complex. Also fixed all linting errors. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

Files in the controller package are already namespaced and don't need "_controller" in their name. Signed-off-by: Jacob Weinstock <[email protected]>

The lines were a bit long for reading. Signed-off-by: Jacob Weinstock <[email protected]>

Signed-off-by: Jacob Weinstock <[email protected]>

jacobweinstock · 2023-05-11T14:46:46Z

@Mergifyio queue

mergify · 2023-05-11T14:46:48Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 219cdd4

jacobweinstock requested review from chrisdoherty4 and pokearu May 5, 2023 23:06

chrisdoherty4 reviewed May 6, 2023

View reviewed changes

jacobweinstock requested a review from chrisdoherty4 May 8, 2023 14:24

chrisdoherty4 approved these changes May 10, 2023

View reviewed changes

chrisdoherty4 added the ready-to-merge Mergify: Ready for Merging label May 10, 2023

jacobweinstock added 5 commits May 10, 2023 09:10

Add linting to Makefile:

521253c

This makes it easier to run the linting. And ci will run the exact same linting. Signed-off-by: Jacob Weinstock <[email protected]>

Add code coverage badge

e67df4f

Signed-off-by: Jacob Weinstock <[email protected]>

Rename files and imports:

b4c46aa

Files in the controller package are already namespaced and don't need "_controller" in their name. Signed-off-by: Jacob Weinstock <[email protected]>

Split lines in TestJobReconcile:

4b6b31e

The lines were a bit long for reading. Signed-off-by: Jacob Weinstock <[email protected]>

jacobweinstock force-pushed the remove-bmcclient-interface branch from 45fdebd to 4b6b31e Compare May 10, 2023 15:11

Fix linting issues

40bb987

Signed-off-by: Jacob Weinstock <[email protected]>

mergify bot merged commit 219cdd4 into tinkerbell:main May 11, 2023

jacobweinstock deleted the remove-bmcclient-interface branch May 11, 2023 14:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove the BMCClient interface, ginkgo, and gomega #102

Remove the BMCClient interface, ginkgo, and gomega #102

jacobweinstock commented May 5, 2023 •

edited

Loading

codecov bot commented May 5, 2023 •

edited

Loading

chrisdoherty4 left a comment

chrisdoherty4 May 6, 2023

jacobweinstock May 8, 2023 •

edited

Loading

chrisdoherty4 May 6, 2023

jacobweinstock May 8, 2023 •

edited

Loading

chrisdoherty4 May 10, 2023

chrisdoherty4 May 6, 2023

jacobweinstock May 8, 2023 •

edited

Loading

chrisdoherty4 May 6, 2023

jacobweinstock May 8, 2023

chrisdoherty4 commented May 10, 2023

mergify bot commented May 10, 2023 •

edited

Loading

jacobweinstock commented May 11, 2023

mergify bot commented May 11, 2023 •

edited

Loading

Remove the BMCClient interface, ginkgo, and gomega #102

Remove the BMCClient interface, ginkgo, and gomega #102

Conversation

jacobweinstock commented May 5, 2023 • edited Loading

Description

Why is this needed

How Has This Been Tested?

How are existing users impacted? What migration steps/scripts do we need?

Checklist:

codecov bot commented May 5, 2023 • edited Loading

Codecov Report

chrisdoherty4 left a comment

Choose a reason for hiding this comment

chrisdoherty4 May 6, 2023

Choose a reason for hiding this comment

jacobweinstock May 8, 2023 • edited Loading

Choose a reason for hiding this comment

chrisdoherty4 May 6, 2023

Choose a reason for hiding this comment

jacobweinstock May 8, 2023 • edited Loading

Choose a reason for hiding this comment

chrisdoherty4 May 10, 2023

Choose a reason for hiding this comment

chrisdoherty4 May 6, 2023

Choose a reason for hiding this comment

jacobweinstock May 8, 2023 • edited Loading

Choose a reason for hiding this comment

chrisdoherty4 May 6, 2023

Choose a reason for hiding this comment

jacobweinstock May 8, 2023

Choose a reason for hiding this comment

chrisdoherty4 commented May 10, 2023

mergify bot commented May 10, 2023 • edited Loading

🛑 The pull request has been removed from the queue

jacobweinstock commented May 11, 2023

mergify bot commented May 11, 2023 • edited Loading

✅ The pull request has been merged automatically

jacobweinstock commented May 5, 2023 •

edited

Loading

codecov bot commented May 5, 2023 •

edited

Loading

jacobweinstock May 8, 2023 •

edited

Loading

jacobweinstock May 8, 2023 •

edited

Loading

jacobweinstock May 8, 2023 •

edited

Loading

mergify bot commented May 10, 2023 •

edited

Loading

mergify bot commented May 11, 2023 •

edited

Loading