Minor readability improvements for e2e test refactor #1855
Conversation
| // nodes. | ||
| func (te *TestEnvironment) GetRandomNodeURI() string { | ||
| r := rand.New(rand.NewSource(time.Now().Unix())) //nolint:gosec | ||
| r := rand.New(rand.NewSource(time.Now().Unix())) //#nosec G404 |
There was a problem hiding this comment.
Rather than skipping the whole linter - I think it's better to be explicit on which rule we are silencing.
| pChainClient := platformvm.NewClient(nodeURI) | ||
| ctx, cancel := context.WithTimeout(context.Background(), e2e.DefaultTimeout) | ||
| validatorIDs, err := pChainClient.SampleValidators(ctx, ids.ID{}, 1) | ||
| validatorIDs, err := pChainClient.SampleValidators(ctx, constants.PrimaryNetworkID, 1) |
There was a problem hiding this comment.
Named constants ❤️
| // will succeed against a persistent network. | ||
| randomShortID, _ := ids.ToShortID(utils.RandomBytes(20)) | ||
| validatorID := ids.NodeID(randomShortID) | ||
| validatorID, err := ids.ToNodeID(utils.RandomBytes(ids.NodeIDLen)) |
There was a problem hiding this comment.
I think ideally we should be able to use ids.GenerateTestNodeID(). Generation of IDs should result in different results for different runs imo.
There was a problem hiding this comment.
I tried using that, but since the ids from is.GenerateTestNodeID() are generated sequentially the same ids will be generated on successive test runs. This was not compatible with a persistent testnet fixture, though if there's a better way to avoid collision I'm open to suggestions.
|
|
||
| // Synchronizes access to test data | ||
| lock sync.Mutex | ||
| TestData |
There was a problem hiding this comment.
I think it's a general best practice to put the lock over the fields that it is locking. I don't recall where I read this... But I think it makes sense... It isn't as important here... But in cases where the struct gets larger then it can improve readability.
| // Type used to marshal/unmarshal a set of test keys for transmission over http. | ||
| type keysDocument struct { | ||
| Keys []*secp256k1.PrivateKey | ||
| Keys []*secp256k1.PrivateKey `json:"keys"` |
There was a problem hiding this comment.
I know the marshaller defaults to the name of the variable... But I think explicitly tagging makes it very clear that we are json marshalling this. Also we typically use camelCase for json rather than CamelCase.
| } | ||
|
|
||
| // Allocate the requested number of keys | ||
| remainingKeys := len(s.FundedKeys) - keyCount |
There was a problem hiding this comment.
I just found the code a lot easier to understand once remainingKeys was a var
| if resp.Body != nil { | ||
| defer resp.Body.Close() | ||
| } | ||
| defer resp.Body.Close() |
There was a problem hiding this comment.
Body is guaranteed to never be nil (and if it wasn't, then the below io.ReadAll(resp.Body) would panic anyway)
| require.ErrorIs(err, tc.expectedError) | ||
|
|
||
| addresses := make([]ids.ShortID, len(keys)) | ||
| for i, key := range keys { | ||
| addresses[i] = key.Address() | ||
| } | ||
| require.Equal(tc.expectedAddresses, addresses) |
There was a problem hiding this comment.
This is a bit odd as it enforces that keys is empty if an error is returned... If we want to remove this requirement than I'd prefer adding a check after the require.ErrorIs(err, tc.expectedError) with if err != nil { return }
| // ZeroSlice sets all values of the provided slice to the type's zero value. | ||
| // | ||
| // This can be useful to ensure that the garbage collector doesn't hold | ||
| // references to values that are no longer desired. | ||
| func ZeroSlice[T any](s []T) { | ||
| for i := range s { | ||
| s[i] = *new(T) | ||
| } | ||
| } |
There was a problem hiding this comment.
I'm not sure if we could've used this in the past (typically we zero out the slice 1 element at a time)... But it seems generally useful.
| "testing" | ||
|
|
||
| ginkgo "github.com/onsi/ginkgo/v2" | ||
|
|
There was a problem hiding this comment.
So I'm clear, the preference is for separating every 3rd party import package?
There was a problem hiding this comment.
Yeah - that's how it's done through the rest of the repo... Really all I want is some super strict linter that we can get behind... but for now consistency is all I can get
Why this should be merged
These are just minor suggestions that I feel would improve readability. None of these changes should (meaningfully) change any logic.
How this works
Small changes.
How this was tested
CI