introduce dell adaptor tests based on a fake dell server #24

rauhersu · 2024-11-14T12:42:30Z

Context

This is a follow-up of the previous loopback plugin effort to increase our test coverage for the plugins initiated with this PR.
Now, this PR targets the dell plugin and as a consequence, a minor refactor of the previous loopback test (mainly dir structure and renaming) has been performed too.

All these new tests are run executing make test from the project root

Notes for the reviewers

2.1 - The loopback-plg dir contains the existing tests for the loopback plugin.
2.2 - The dell-plg dir contains the new test code, targeting the dell plugin.
2.3 - The assets dir contains new manifests needed by the dell plugin for the new testing scenario. It contains 'common logic' used by both plugins.
2.4 - The dell-server dir contains code generated on parsing the Dell swagger (drop2) specification, tweaked by @donpenney

This PR is only focused on a rest scenario for the dell plugin, specifically the authentication token request to the dell server, but this dell test suite has been designed to be scalable and might test the whole rest flow plus a complete reconciliation if no architectural constraint prevents it (i.e: a real cluster is needed).
Technicalities

3.1 - Envtest is used to setup a minimal control plane. No cluster is needed.
3.2 - A goroutine is triggered to start up the fake dell server while the test is run in the main one - here -
3.3 - The fake dell server exposes all API routes - here - but only the 'get token' scenario is under test so far - here - . Posterior MRs might cover the whole API. Also, every response is mocked to test the Dell plugin behaviour on a test basis.
Other

4.1 - While writing this I have just noticed we need to update our manifests according to Tao's latest changes: CNF-15609: Enhance Loopback Plugin for Node Pool Configuration Changes #23 we might hold this PR to merge that before and update this PR accordingly.
4.2 - @donpenney , I see a lot of 'errors' indicated by the linter (wrapcheck) for the generated code (from the swagger file)
4.3 - It might be interesting to test this 'get token' scenario provisioning the configmap with a cabundle; so far I have not included this.

rauhersu · 2024-11-14T12:52:14Z

/hold

We might want to merge Tao's MR first and update this one with new manifests structure

donpenney · 2024-11-14T13:16:39Z

#23 has been merged
#19 should be merged shortly

donpenney · 2024-11-14T15:59:52Z

I would recommend using loopback-adaptor and dell-adaptor instead of plg to be consistent with the naming convention in the plugin architecture

donpenney · 2024-11-14T16:01:37Z

Please add a Makefile target for generating the test server code. I would also recommend putting it in a generated directory for clarity. Make sure to also add an entry to the .gitattributes file to set this folder as containing generated code.

donpenney · 2024-11-14T16:05:00Z

We should also be able to edit the .golangci-lint.yaml config file to ignore the generated code, and could consider ignoring test code as well

test/plugins/assets/manifests/dell-hm.tmpl

donpenney · 2024-11-14T16:20:02Z

I would recommend using loopback-adaptor and dell-adaptor instead of plg to be consistent with the naming convention in the plugin architecture

Similarly, please rename test/plugins to test/adaptors. You could then use loopback and dell-hwmgr under here to align with the folders under adaptors

rauhersu · 2024-11-15T11:28:39Z

Thanks for the feedback, @donpenney

Comments about my new push below.

I would recommend using loopback-adaptor and dell-adaptor instead of plg to be consistent with the naming convention in the plugin architecture

Done

Please add a Makefile target for generating the test server code. I would also recommend putting it in a generated directory for clarity. Make sure to also add an entry to the .gitattributes file to set this folder as containing generated code.

I am only re-using your types.go from the dell adaptor folder, no extra Makefile target is needed.

The reason is that the server generated code (the http dispatch infra) does not favour our intend to mock the dispatch functions dynamically on a test basis, as I am doing for our tests: here and here . I believe I am able to provide a better customisation on the http side instead, based on gorilla/mux . See muxinfra.go.

Note that my muxinfra is pretty standard golang code leveraging mux, and orthogonal to openapi changes, so I am expecting not to change this for any reason.

Otoh, if we need to update our openapi spec, on the pure server side code I am only expecting to update this routes function when updating endpoints for the corresponding test.

We should also be able to edit the .golangci-lint.yaml config file to ignore the generated code, and could consider ignoring test code as well

Have not done it so far but we might simply add the nolint annotation for api package, right ? As that file will be autogenerated and newer files will override this, can we do it on a file basis instead - types.go - ?

I'd recommend using hwmgr in the filename rather than hm for consistency

Done

Similarly, please rename test/plugins to test/adaptors. You could then use loopback and dell-hwmgr under here to align with the folders under adaptors

Done

rauhersu · 2024-11-15T12:03:51Z

/unhold

donpenney · 2024-11-15T13:48:35Z

test/adaptors/dell-hwmgr/dell-server/mux-infra/muxinfra.go

+	"github.com/gorilla/mux"
+	"log"
+	"net/http"
+	"time"


golangci-lint wants the system imports first, with external imports separated

Suggested change

"github.com/gorilla/mux"

"log"

"net/http"

"time"

"log"

"net/http"

"time"

"github.com/gorilla/mux"

rauhersu · 2024-11-18T17:29:57Z

@donpenney , see this new commit added:

5f994c9

No new functionality, two improvements added:

the required CRDs cloned by git add a 'retry' mechanism useful to deal with glitches. See crds.go.
the fake dell server generated code has been integrated with less changes for the developer to extend: muxinfra.go has been deleted, see the attached README containing all the details.

test/adaptors/README.md

donpenney · 2024-11-18T18:48:56Z

test/adaptors/dell-hwmgr/suite_test.go

+
+	// fetch required CRD
+	crdPath := filepath.Join(tmpDir, crdRepoName)
+	err := crds.GetRequiredCrdsFromGit(crdRepo, crdPath)


Is this just getting the main/latest content? The go.mod locks us to a specific version of the APIs from oran-o2ims. We'd need to do the same here, to ensure that the CRD structure aligns with what's expected by the plugin code.

Yes, it is getting the latest. How do we take the decision to update to a specific oran-o2ims api from the code perspective ?

From the testing side, using this git mechanism, we are no more granular than a branch. I am guessing we are not going have a git tag to sync to either. Not sure if can derive the associated CRD file then: go.mod version --> crd file

If we just need a branch as a parameter, it is pretty straightforward; we will just need to update this file on a release branch basis (release-4.18, release-4.19, ...)

This has been addressed as part of the new changes, see the README: Testing dependencies and the new code in utils.go

donpenney · 2024-11-18T18:49:25Z

test/adaptors/dell-hwmgr/suite_test.go

+		ctx, cancel = context.WithCancel(
+			context.Background())
+		err := server.Shutdown(ctx)
+		Expect(err).NotTo(HaveOccurred(), "failed to stop dell server")


Suggested change

Expect(err).NotTo(HaveOccurred(), "failed to stop dell server")

Expect(err).NotTo(HaveOccurred(), "failed to stop test server")

donpenney · 2024-11-27T08:38:05Z

Please fix remaining linter warnings

test/adaptors/crds/crds.go

donpenney · 2024-11-27T09:08:49Z

test/adaptors/crds/crds.go

+
+// GetRequiredCRDsFromGit clones an oran-o2ims repo in a specific path and checks out a specific commit
+// to fetch required CRDs from there
+func GetRequiredCRDsFromGit(crdRepo string, commit string, crdPath string) error {


Suggested change

func GetRequiredCRDsFromGit(crdRepo string, commit string, crdPath string) error {

func GetRequiredCRDsFromGit(crdRepo, commit, crdPath string) error {

test/adaptors/crds/crds.go

donpenney · 2024-11-27T09:09:11Z

test/adaptors/crds/crds.go

+	backoff := retry.NewFibonacci(1 * time.Second)
+	err := retry.Do(ctx, retry.WithMaxRetries(retries, backoff), cloneFn)
+	if err != nil {
+		return fmt.Errorf("failed to clone repo:%s, err:%v", crdRepo, err)


Suggested change

return fmt.Errorf("failed to clone repo:%s, err:%v", crdRepo, err)

return fmt.Errorf("failed to clone repo:%s, err:%w", crdRepo, err)

donpenney · 2024-11-27T09:09:21Z

test/adaptors/crds/crds.go

+	// checkout the specific commit
+	w, err := r.Worktree()
+	if err != nil {
+		return fmt.Errorf("failed to get git worktree repo:%s, err:%v", crdRepo, err)


Suggested change

return fmt.Errorf("failed to get git worktree repo:%s, err:%v", crdRepo, err)

return fmt.Errorf("failed to get git worktree repo:%s, err:%w", crdRepo, err)

test/adaptors/crds/crds.go

A minor refactor of the existing loopback tests has been peformed too

rauhersu · 2024-11-27T12:02:16Z

Thanks once again, @donpenney . Regarding the linter comments, I have had some issues because of the linter tool already included. Cc'ed you over here: https://redhat-internal.slack.com/archives/C07A9NFCS01/p1732707795237589

openshift-ci · 2024-11-27T12:08:53Z

@rauhersu: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ci-bundle-operator-bundle	`c7a158a`	link	true	`/test ci-bundle-operator-bundle`
ci/prow/ci-job	`c7a158a`	link	true	`/test ci-job`
ci/prow/images	`c7a158a`	link	true	`/test images`

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

donpenney · 2024-11-27T12:14:48Z

/lgtm
/approve

openshift-ci · 2024-11-27T12:14:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: donpenney

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:

~~OWNERS~~ [donpenney]

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

openshift-ci bot requested a review from browsell November 14, 2024 12:42

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2024

rauhersu force-pushed the main.dell.squash branch from 432e12f to 66dff7e Compare November 14, 2024 12:50

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 14, 2024

rauhersu force-pushed the main.dell.squash branch from 66dff7e to dbb986f Compare November 14, 2024 13:14

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 14, 2024

donpenney reviewed Nov 14, 2024

View reviewed changes

test/plugins/assets/manifests/dell-hm.tmpl Outdated Show resolved Hide resolved

rauhersu force-pushed the main.dell.squash branch from dbb986f to 86829aa Compare November 15, 2024 10:49

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 15, 2024

rauhersu force-pushed the main.dell.squash branch from 86829aa to f1ed155 Compare November 15, 2024 10:58

rauhersu changed the title ~~introduce dell plugin tests with a fake dell server~~ introduce dell adaptor tests based on a fake dell server Nov 15, 2024

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 15, 2024

donpenney reviewed Nov 15, 2024

View reviewed changes

rauhersu force-pushed the main.dell.squash branch from 5f994c9 to 783149d Compare November 18, 2024 17:58

donpenney reviewed Nov 18, 2024

View reviewed changes

rauhersu force-pushed the main.dell.squash branch 5 times, most recently from b1c1a15 to ab9a828 Compare November 21, 2024 18:02

donpenney reviewed Nov 27, 2024

View reviewed changes

introduce dell adaptor tests based on a fake dell server ...

c7a158a

A minor refactor of the existing loopback tests has been peformed too

rauhersu force-pushed the main.dell.squash branch from ab9a828 to c7a158a Compare November 27, 2024 11:55

openshift-ci bot assigned donpenney Nov 27, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 27, 2024

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 27, 2024

donpenney merged commit 3880b1b into openshift-kni:main Nov 27, 2024
4 of 8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

introduce dell adaptor tests based on a fake dell server #24

introduce dell adaptor tests based on a fake dell server #24

rauhersu commented Nov 14, 2024 •

edited

Loading

rauhersu commented Nov 14, 2024 •

edited

Loading

donpenney commented Nov 14, 2024

donpenney commented Nov 14, 2024

donpenney commented Nov 14, 2024

donpenney commented Nov 14, 2024

donpenney commented Nov 14, 2024

rauhersu commented Nov 15, 2024 •

edited

Loading

rauhersu commented Nov 15, 2024

donpenney Nov 15, 2024

rauhersu commented Nov 18, 2024 •

edited

Loading

donpenney Nov 18, 2024

rauhersu Nov 19, 2024 •

edited

Loading

rauhersu Nov 21, 2024 •

edited

Loading

donpenney Nov 18, 2024

rauhersu Nov 21, 2024

donpenney commented Nov 27, 2024

donpenney Nov 27, 2024

donpenney Nov 27, 2024

donpenney Nov 27, 2024

rauhersu Nov 27, 2024

rauhersu commented Nov 27, 2024

openshift-ci bot commented Nov 27, 2024

donpenney commented Nov 27, 2024

openshift-ci bot commented Nov 27, 2024

	Expect(err).NotTo(HaveOccurred(), "failed to stop dell server")
	Expect(err).NotTo(HaveOccurred(), "failed to stop test server")

	func GetRequiredCRDsFromGit(crdRepo string, commit string, crdPath string) error {
	func GetRequiredCRDsFromGit(crdRepo, commit, crdPath string) error {

	return fmt.Errorf("failed to clone repo:%s, err:%v", crdRepo, err)
	return fmt.Errorf("failed to clone repo:%s, err:%w", crdRepo, err)

	return fmt.Errorf("failed to get git worktree repo:%s, err:%v", crdRepo, err)
	return fmt.Errorf("failed to get git worktree repo:%s, err:%w", crdRepo, err)

introduce dell adaptor tests based on a fake dell server #24

introduce dell adaptor tests based on a fake dell server #24

Conversation

rauhersu commented Nov 14, 2024 • edited Loading

rauhersu commented Nov 14, 2024 • edited Loading

donpenney commented Nov 14, 2024

donpenney commented Nov 14, 2024

donpenney commented Nov 14, 2024

donpenney commented Nov 14, 2024

donpenney commented Nov 14, 2024

rauhersu commented Nov 15, 2024 • edited Loading

rauhersu commented Nov 15, 2024

Choose a reason for hiding this comment

rauhersu commented Nov 18, 2024 • edited Loading

Choose a reason for hiding this comment

rauhersu Nov 19, 2024 • edited Loading

Choose a reason for hiding this comment

rauhersu Nov 21, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

donpenney commented Nov 27, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauhersu commented Nov 27, 2024

openshift-ci bot commented Nov 27, 2024

donpenney commented Nov 27, 2024

openshift-ci bot commented Nov 27, 2024

rauhersu commented Nov 14, 2024 •

edited

Loading

rauhersu commented Nov 14, 2024 •

edited

Loading

rauhersu commented Nov 15, 2024 •

edited

Loading

rauhersu commented Nov 18, 2024 •

edited

Loading

rauhersu Nov 19, 2024 •

edited

Loading

rauhersu Nov 21, 2024 •

edited

Loading