refactor DevicePortService to use added PortService

[displague]

While attempting to implement ports for equinix/metal-cli#58 (to ease manual testing of #239), I found that packet port bond --id uuid was not straight-foward to implement with Packngo.

Packngo currently expects all port operations to be predicated by a device id. This model does not accurately represent the Equinix Metal API.

In this PR, a new PortService is added (refactored from DevicePortService methods) and the DevicePortService now relies on the PortService rather than duplicating code.

DevicePortService is deprecated per #230. Users may wish to migrate those helpers to other projects where they are consumed if we still need them.

The existing ports.go was renamed to device_ports.go, and the new service was added to ports.go. I have also moved the Port related types over to ports.go to clear the way for device_ports.go removal.

PortServiceOp.Assign, PortServiceOp.AssignNative, and PortServiceOp.Unassign take portID, vlanID string parameters instead of a single PortAssignmentRequest. This is to make these methods more consistent with others that take portID as a first parameter. The second parameter could continue to be a PortAssignmentRequest and we may wish to change the second parameter to a struct if more than one option is ever offered for these methods. Today, there is only one request parameter for these methods and the portID is a URL parameter.

[t0mk]

Hi @displague, good idea. The port operations take the device ID because in the past, the Port direct url didn't work: #132

closes #132

[t0mk]

I'm happy to test once there will be some acceptance tests to run.

[displague]

I'm happy to test once there will be some acceptance tests to run.

We should have matching coverage based on the existing tests for the refactored DevicePortService functions that are now calling the PortService functions.

I'll look for ways to improve coverage.

At the very least, I should add a test for PortService.Get which has no coverage.

[displague]

I'll also duplicate the DevicePortService tests for PortService, which will make it easier to deprecate DevicePortService functions in the future while maintaining coverage.

[displague]

I'll also duplicate the DevicePortService tests for PortService, which will make it easier to deprecate DevicePortService functions in the future while maintaining coverage.

I didn't duplicate the DevicePort acceptance tests because of how long they take and how complex they are. If we want to remove those functions later we can adapt the acceptance tests at that time, perhaps move DevicePorts helpers into their own package.

I ended up adding an integration acceptance test for PortServiceOp.Get and full coverage tests for the code under test in ports.go (verify that the HTTP methods and URLs are correct and make sure we get the expected response when a fetch succeeds or fails.

[t0mk]

Overall looks good. Thanks for separating device IDs from the ports ops, commenting fields in the Port struct, and for cleaning the assign functions - I guess the POST body json doesn't need the port ID if it;s in the URL.

I still think unit tests with mocks are useless, as the API often changes without us knowing. You never know when a unit test is outdated. You always know when an acceptance test is outdated when you run it, and it fails because of missing field or an old URL. But I don't mind keeping the unit tests.

It would be good to have acceptance tests for more of the reimplemented operations.

[t0mk]

maybe put this func to utils.go

[displague]

That makes sense. I took a look and reverse is only used in _test builds, and only in ports_test.go.
I think we can keep it here for now.

utils.go should get refactored away too, some of the functions that are exported should be private/internal and the stale constants are unused and should be removed.

[displague]

the API often changes without us knowing. You never know when a unit test is outdated.

As I see it, the unit tests ensure that the few lines of code involved in each method do what they are intended to do. Passing these tests means that we haven't broken these assumptions.

As an API client, this verifies that the client works.

The API itself is the concern of other teams. If the API is broken, we can identify and report those problems, but we are in a poor position to solve API discrepancies or errors from this client.

The scope of this client (and its risks) is smallest if it stays unopinionated and acts as a thin Go wrapper to the HTTP endpoints and JSON objects, especially if that can follow what is documented in the Swagger spec.

The logic that we test is the API handling logic. If packngo invents logic about how to interact with the API to perform larger scoped operations, then we need to test that logic based on the expected and potential inputs and outputs at each code branch. We can test this logic with mocks. It's not practical, in my opinion, to spin up a new device to test an alternate condition in a large function when we could mock the condition toggling values to get the same coverage.

Most of the packngo functions have little logic (with some exceptions https://goreportcard.com/report/github.com/packethost/packngo#gocyclo). Code coverage providing unit tests only need to ensure the API requests have the expected URL, verify that errors are handled, and ensure receiving objects are affected. We generally don't modify the request parameters or make any logical branches based on parameters. Packngo blindly sends to the API what the user passed to each method.

It would be good to have acceptance tests for more of the reimplemented operations.

The acceptance tests that we currently have for DevicePorts are effectively acceptance testing the Ports functions because the DevicePorts functions were refactored to call all of the Ports functions.

The one function that was not called by DevicePorts is PortServiceOp.Get, which now has its own acceptance test.

I'm taking the view that the coverage we have now is well enough for these acceptance tests.

---

I wondered if there are benefits to moving the acceptance tests into a separate package, I haven't found examples of this.

In that search, I stumbled upon https://stackoverflow.com/questions/25965584/separating-unit-tests-and-integration-tests-in-go. The git test --tags=integration (opt-in vs -short opt-out) approach seems nice. The environment variable checking wouldn't be needed and we could keep acceptance tests separate from unit tests.

[displague]

We should add unit-tests that ensure every branch in DoRequest, NewClient, NewRequest, and other packngo.go functions work as intended since these are at the core of every FooServiceOp.