Skip to content
This repository has been archived by the owner on Jun 21, 2024. It is now read-only.

Introduces the govcr test recorder #193

Merged
merged 6 commits into from
Sep 14, 2020

Conversation

displague
Copy link
Contributor

@displague displague commented Sep 10, 2020

Introduces govcr test recorder, disabled by default. "play" mode should become the default once we have recorded sanitized fixtures for all tests.

Also fixes golanglint-ci linter warnings. (see dominikh/go-tools#503 (comment) regarding the for{switch{case<-time.Tick changes)

Also deprecates some precarious public variables: https://github.com/packethost/packngo/pull/193/files#diff-7d1c2a3334601b6c1958aae0a594cba8

devices_test.go Outdated Show resolved Hide resolved
devices_test.go Outdated Show resolved Hide resolved
virtualnetworks_test.go Show resolved Hide resolved
utils.go Outdated
@@ -9,33 +9,46 @@ import (

var (
timestampType = reflect.TypeOf(Timestamp{})
Facilities = []string{

// Facilities DEPRECATED See Facilities.List
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at Facilities.List and saw nothing mentioned there.
We have the list of facilities here to speed up test for whether a facility code is correct.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dtto for All the other resource listings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll change "See" to "Use". I didn't mean for users to find more explanation in the Facilities.List documentation, rather I meant they should not use this static list in favor of the dynamic one.

If we need this for packngo tests we should put it in utils_test.go (or more likely packngo_test.go). FWIW, I didn't see Facilities being referenced at all in this project.

If non-packngo packages are using this variable it's more problematic, because this is an outdated and incomplete list.

devices_test.go Show resolved Hide resolved
@t0mk
Copy link
Contributor

t0mk commented Sep 14, 2020

@displague I looked at the code, and wrote some comments, only request for change would be rebase to master, so that the provisoiing events check goes away.

I will now run the tests and see how the recording and playing works.

@t0mk
Copy link
Contributor

t0mk commented Sep 14, 2020

Since this PR changes tests, would you mind changing the size iin VolumeCreateRequest from 10 to 100 in all the relevant files (grep VolumeCreate *test.go). 100 is the new minimum size.

@t0mk
Copy link
Contributor

t0mk commented Sep 14, 2020

I run tests for email, volume and device and tried to record and play, it worked OK.

The TestAccDeviceBasic test was doing the delays of the API and I honestly don't understand where the delays came from, or how the duration of the delays were recorded.

@displague
Copy link
Contributor Author

The TestAccDeviceBasic test was doing the delays of the API and I honestly don't understand where the delays came from, or how the duration of the delays were recorded.

HTTP headers return the time that a client should wait before retrying, client-side delays would be against the timer and would wait however long the server requested (between recorded/replayed requests). When we sanitize fixtures we can reduce the delay that the server requested (this can be done in the same place the X-Auth-Token is sanitized).

@t0mk
Copy link
Contributor

t0mk commented Sep 14, 2020

@displague Cool! Otherwise this PR looks really good. You introduced a new additional way of testing without breaking anything, that can only be appreciated.

@displague displague merged commit 75dd10f into equinixmetal-archive:master Sep 14, 2020
@displague displague deleted the test-recorder-init branch September 14, 2020 18:58
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants