start adding unit tests for ironic.ValidateManagementAccess by dhellmann · Pull Request #647 · metal3-io/baremetal-operator

dhellmann · 2020-09-23T13:51:55Z

No description provided.

dhellmann · 2020-09-23T14:27:19Z

/test-integration

dhellmann · 2020-09-24T15:37:30Z

I would like to freeze non-essential changes for a few days to try to land #650 without having to rebase it, because rebasing will mean redoing the work from scratch.

/hold

dhellmann · 2020-10-01T15:25:22Z

#655 has merged

/hold cancel

dhellmann · 2020-10-01T15:25:32Z

/test-integration

dhellmann · 2020-10-09T18:38:22Z

I've rebased this on the work @andfasano did in #649

/test-integration

andfasano · 2020-10-12T09:52:04Z

 			hostName:       "name",
 			provisioningID: "uuid",
-			ironic: testserver.New(t, "ironic").NotFound("/v1/nodes/name").
+			ironic: testserver.NewIronic(t).NotFound("/v1/nodes/name").


NotFound as a generic method is ok, an alternative could be to use WithNodeError, ie:

testserver.NewIronic(t).WithNodeError("name", http.StatusNotFound).WithNodeError("uuid", http.StatusNotFound)

On a second pass, noticed you've added also a NoNode, which could be even better for simplifying mock's usage since it requires just the node name

Good point. Done.

andfasano · 2020-10-12T10:04:20Z

+		bodyRaw, err := ioutil.ReadAll(r.Body)
+		if err != nil {
+			m.logRequest(r, fmt.Sprintf("ERROR: %s", err))
+			http.Error(w, fmt.Sprintf("%s", err), 500)


Minor point, http.StatusInternalServerError could be more readable

andfasano · 2020-10-12T10:04:36Z

+		response := fmt.Sprintf("{\"uuid\": \"%s\", %s", uuid, strings.TrimLeft(body, "{"))
+
+		w.Header().Set("Content-Type", "application/json")
+		w.WriteHeader(201)


And http.StatusCreated here

andfasano · 2020-10-12T10:17:31Z

+	assert.Equal(t, "", result.ErrorMessage)
+	assert.NotEqual(t, "", host.Status.Provisioning.ID)
+	assert.Equal(t, ironic.CreatedNodes[0].UUID, host.Status.Provisioning.ID)
+}


(Not blocking) I noticed that the three test cases have been split into three different test methods (TestValidateManagementAccessNoMAC, TestValidateManagementAccessMACOptional and TestValidateManagementAccessCreateNode) sharing a pretty similar setup code.
Maybe it's not worth in this case, but to transform the three methods in a list of cases in a single test method it could be useful to "upgrade" the makeHost() factory method to a proper host builder object with fluent interface, so that also the host could become a parameter of the cases list (and of course also the expectations code will need to be adjusted).

Given that the ValidateManagementAccess is a pretty big method to be covered, the existing node and/or credentials change case will be added within this PR?

The setup portions of the functions are similar, but the checks were different enough that it seemed like it would be clearer to just write separate functions this time, instead of having more complicated logic to decide which tests to apply in each case.

In terms of test coverage, I think this PR is big enough and I would like to merge it before finishing the tests for ValidateManagementAccess.

Yes, makes sense

andfasano · 2020-10-12T10:21:25Z

 }
+
+// Implements provisioner.EventPublisher to swallow events for tests.
+func nullEventPublisher(reason, message string) {}


Nice one, we could introduce a set of different hard-coded publishers (ie null, std) to be reused in all the tests to remove some other lines of code

andfasano · 2020-10-12T10:24:37Z

+		})
+
+		// hackily add uuid to the json response by inserting it to the front of the string
+		response := fmt.Sprintf("{\"uuid\": \"%s\", %s", uuid, strings.TrimLeft(body, "{"))


Just as a confirmation, is it fine to return all the fields sent in the body request as a response? From a quick glance on the API here should be fine. Maybe in future we could need something more sophisticated, but for the current test seems ok

Yes, my understanding is the API is supposed to return the node, which may include new fields (as with UUID here) and should include the data passed to us. A better implementation would extract the input into a nodes.Node, add the UUID, then marshal it back to JSON for the response, but I'm trying to keep things simple.

andfasano

Current changes look fine to me

metal3-io-bot · 2020-10-13T15:35:09Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andfasano, dhellmann

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [dhellmann]

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

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Make the BMC access type registration function public so we can add fixture types in tests for the ironic provisioner. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Add a testbmc type so the ironic tests that create hosts can use a test:// URL for the BMC address. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Verify that ValidateManagementAccess() reports an error when the driver needs a bootMACAddress and the host does not have one. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Verify that ValidateManagementAccess() does not report an error when the host has no boot MAC and the BMC does not require one. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Add CreateNodes() method to IronicMock to simulate creating a node. Add a test for ValidateManagementAccess that triggers creating a node and verifies that the host status is updated with the UUID of the new node. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

Look at the node struct we're given and register the responses for both UUID and Name, based on which values are filled in. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann · 2020-10-15T13:12:00Z

/test-integration

Rebased to get the make docker fix from #647

dhellmann · 2020-10-15T13:12:53Z

/cc @asalkeld

andfasano · 2020-10-15T14:56:56Z

+	if node.UUID != "" {
+		m.ResponseJSON("/v1/nodes/"+node.UUID, node)
+	}
+	if node.Name != "" {


I have a couple of question on this change:

Is it a valid case to have the UUID unset and Name set?

When both UUID and Name are set, will the ServeMux.Handle panic?

A real ironic service wouldn't have a node without a UUID, but in the tests we sometimes want the node to only be available via one or the other value to simulate the node having settings different from what the provisioner expects.

I'm not sure why the handler would panic? The endpoints are different (one looks up by UUID, the other by name).

In case Name == UUID, but I think it's a trivial case

Ah, yes, I didn't consider that. I think it's OK for the test to panic in that situation, since that doesn't seem like something we would expect to encounter in real data. I could add a test to protect against it, if you think it's worth it?

I don't think it's worth it, and agree it's fine to panic in such situation

asalkeld

/lgtm

metal3-io-bot · 2020-10-19T06:41:53Z

@asalkeld: adding LGTM is restricted to approvers and reviewers in OWNERS files.

Details

In response to this:

/lgtm

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/test-infra repository.

andfasano · 2020-10-19T07:10:04Z

I'm also fine with the changes

zaneb · 2020-10-20T18:43:46Z

+func init() {
+	bmc.RegisterFactory("test", newTestBMCAccessDetails, []string{})
+	bmc.RegisterFactory("test-needs-mac", newTestBMCAccessDetails, []string{})
+}


Would it be safer to register these explicitly in tests rather than it happening as a side-effect here?

Probably. On the other hand, all of the rest of the registry stuff expects to be invoked from an init() function like this, the registry is global state without a way to reset right now, and I'm not sure what happens if the same name is registered more than once.

I would be happy to consider making the registry more accepting of dynamic use in a separate PR.

zaneb · 2020-10-20T18:53:16Z

/lgtm

metal3-io-bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 23, 2020

dhellmann force-pushed the unit-test-validate-management-access branch from 89806b8 to d791e5b Compare September 23, 2020 14:27

metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 24, 2020

dhellmann mentioned this pull request Sep 24, 2020

start adding unit tests for Ironic.InspectHardware #649

Merged

dhellmann force-pushed the unit-test-validate-management-access branch from d791e5b to ba1e7d0 Compare October 1, 2020 15:19

metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 1, 2020

dhellmann force-pushed the unit-test-validate-management-access branch from ba1e7d0 to 28d2d40 Compare October 9, 2020 18:37

dhellmann requested a review from andfasano October 9, 2020 18:38

dhellmann force-pushed the unit-test-validate-management-access branch from 28d2d40 to 4beff98 Compare October 9, 2020 19:07

andfasano reviewed Oct 12, 2020

View reviewed changes

dhellmann force-pushed the unit-test-validate-management-access branch from 4beff98 to 2f1cebc Compare October 13, 2020 14:20

dhellmann requested a review from andfasano October 13, 2020 14:20

dhellmann force-pushed the unit-test-validate-management-access branch from 2f1cebc to f1d8c9d Compare October 13, 2020 14:33

andfasano approved these changes Oct 13, 2020

View reviewed changes

dhellmann added 7 commits October 15, 2020 09:10

use helper to create test ironic server

10ecde8

Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

make bmc.RegisterFactory public

1c892aa

Make the BMC access type registration function public so we can add fixture types in tests for the ironic provisioner. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

require a valid bmc address value for all hosts

49db67c

Add a testbmc type so the ironic tests that create hosts can use a test:// URL for the BMC address. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

ensure ironic provisioner requires boot mac when driver does

5233b6d

Verify that ValidateManagementAccess() reports an error when the driver needs a bootMACAddress and the host does not have one. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

ensure ironic provisioner makes boot mac optional when driver does

4dd9c74

Verify that ValidateManagementAccess() does not report an error when the host has no boot MAC and the BMC does not require one. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

ironic mock: register nodes by uuid and/or name

c167d52

Look at the node struct we're given and register the responses for both UUID and Name, based on which values are filled in. Signed-off-by: Doug Hellmann <dhellmann@redhat.com>

dhellmann force-pushed the unit-test-validate-management-access branch from f1d8c9d to c167d52 Compare October 15, 2020 13:11

dhellmann requested a review from andfasano October 15, 2020 13:12

metal3-io-bot requested a review from asalkeld October 15, 2020 13:12

andfasano reviewed Oct 15, 2020

View reviewed changes

asalkeld reviewed Oct 19, 2020

View reviewed changes

zaneb reviewed Oct 20, 2020

View reviewed changes

metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 20, 2020

metal3-io-bot merged commit d055c59 into metal3-io:master Oct 20, 2020

Conversation

dhellmann commented Sep 23, 2020

Uh oh!

dhellmann commented Sep 23, 2020

Uh oh!

dhellmann commented Sep 24, 2020

Uh oh!

dhellmann commented Oct 1, 2020

Uh oh!

dhellmann commented Oct 1, 2020

Uh oh!

dhellmann commented Oct 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andfasano left a comment

Choose a reason for hiding this comment

Uh oh!

metal3-io-bot commented Oct 13, 2020

Uh oh!

dhellmann commented Oct 15, 2020

Uh oh!

dhellmann commented Oct 15, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

asalkeld left a comment

Choose a reason for hiding this comment

Uh oh!

metal3-io-bot commented Oct 19, 2020

Uh oh!

andfasano commented Oct 19, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zaneb commented Oct 20, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants