test(deviceauth): port device auth tests to integration:ng - #2074
test(deviceauth): port device auth tests to integration:ng#2074p-targowicz wants to merge 1 commit into
Conversation
d19a365 to
0754f85
Compare
4098ae5 to
c6c45f3
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
294aafd to
75cde9a
Compare
c6c45f3 to
dd74371
Compare
75cde9a to
4214e83
Compare
dd74371 to
f3755c7
Compare
bahaa-ghazal
left a comment
There was a problem hiding this comment.
looks good, would appreciate if could use the same style as in here
f3755c7 to
c62261f
Compare
d905f66 to
be3ac45
Compare
781170a to
90db24e
Compare
bahaa-ghazal
left a comment
There was a problem hiding this comment.
LGTM!
I would also like @merlin-northern to take a look at it to be more safe I didn't miss anything
absolutely @merlin-northern please take a look whenever you have some spare time 🙏 |
125cadc to
6d91e79
Compare
90db24e to
35b08e7
Compare
Preauthorization, device and authset management, and the auth request flow across all six key types, ported from test_devauth.py, which keeps running in parallel. Signed-off-by: Patryk Targowicz <patryk.targowicz@northern.tech>
35b08e7 to
d1c4932
Compare
merlin-northern
left a comment
There was a problem hiding this comment.
okey, I would like to discuss during the MSTM the policy on rewriting the tests. let's have a decision and a defined approach. one way is: leave the python tests as legacy, develop new tests in the new framework, and every time we modify something we write/update the test case in tests-ng. also I would like to discuss the approach to AI generated code and set the policy for that.
I am not blocking this, but I would kindly request no more porting before we discuss&agree as mentioned above.
| "github.com/mendersoftware/mender-server/tests/runner/tests/common" | ||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
| "github.com/stretchr/testify/suite" |
There was a problem hiding this comment.
| "github.com/stretchr/testify/suite" | |
| "github.com/google/uuid" | |
| "github.com/stretchr/testify/assert" | |
| "github.com/stretchr/testify/require" | |
| "github.com/stretchr/testify/suite" | |
| "github.com/mendersoftware/mender-server/pkg/api/client" | |
| "github.com/mendersoftware/mender-server/pkg/utils/types" | |
| "github.com/mendersoftware/mender-server/services/deviceauth/model" | |
| "github.com/mendersoftware/mender-server/tests/runner/tests/common" |
nitpicking.
| s.Run("Ok", s.preauthOk) | ||
| s.Run("FailDuplicate", s.preauthFailDuplicate) | ||
| s.Run("FailBadRequest", s.preauthFailBadRequest) |
There was a problem hiding this comment.
looking for instance at the compareDevs and preauthOk it is impossible to distinguish between the test case and the helper function. there should be a clean distinction.
| KP *common.KeyPair | ||
| } | ||
|
|
||
| type devFixture struct { |
There was a problem hiding this comment.
why is it called a fixture?
| // docker_compose_environment.go), so callers that need to compare the full | ||
| // listing/paging/count against a locally tracked device set must pass ids | ||
| // to filter out devices left behind by other tests/suites. | ||
| func (s *DevauthManagementV2Suite) listDevices(ctx context.Context, status string, page, perPage int32, ids ...string) ([]client.Device, error) { |
There was a problem hiding this comment.
we have been there. the python tests have 10 tons of helpers, abstraction, and stuff that mask the true purpose of the test and makes it impossible to read now and insanely difficult for future us to maintain. this is what the python tests proved: the insane design patterns and strange abstractions have no place in the test code. the test code should be an easy to read and super simple to understand reference for how the product is expected to work, even so at the cost of duplicating code. what we did with the tests-ng was to write the test cases calling directly the pkg/client everywhere where possible (AcceptWait func is one important exception, about which below). this makes the test maintainable, why? because in 10 years someone reading the test code sees DeviceAuthManagementListDevices().Execute() and immediately sees what is being tested, and why, without jumps to definitions in other places. there are cases where we need to have a common code as is the case with accepting a device (see AcceptWait) so some balance is eeded.
| func (s *DevauthManagementV2Suite) getDevice(ctx context.Context, id string) (*client.Device, error) { | ||
| dev, _, err := s.APIClient.DeviceAuthenticationManagementAPIAPI. | ||
| DeviceAuthManagementGetDevice(ctx, id).Execute() | ||
| return dev, err | ||
| } |
There was a problem hiding this comment.
see below, this is another helper that makes things worse.
| // object. The generated client can't build this (identity_data is | ||
| // typed as an object), so this one goes over a raw request. | ||
| body, err := json.Marshal(map[string]any{ | ||
| "identity_data": `{"mac": "foo"}`, |
There was a problem hiding this comment.
this really looks like a trap and a riddle, takes some time to understand that the `` make all the difference.
| "pubkey": kp.PublicKeyPEM(), | ||
| }) | ||
| require.NoError(err) | ||
| resp, err := common.RawRequest(ctx, s.APIClient, http.MethodPost, "/api/management/v2/devauth/devices", body) |
There was a problem hiding this comment.
hardcoded endpoint, and method, has nothing to do with the code or spec. I think the correct way to inject the bad payload is to override RoundTrip in the http.RoundTripper interface and set a new HTTPClient in the openapi client configuration with custom Transport.
Part of the suite-by-suite python-to-Go port (see #2079). Ports
test_devauth.py: preauthorization, device and authset management, auth
requests across RSA, four ECDSA curves and Ed25519. The python tests
keep running in parallel. Builds on #2079.