Update HTTP tests #275
Update HTTP tests #275
Conversation
Details: - Introduce global HTTP service instance and test coordination for concurrent execution. - Adjust timeout settings and improve logging based on environment variables.
- Introduced `NetworkName` function to identify the network based on genesis validators root. - Added unit tests for the `NetworkName` function to ensure correct identification of known networks. - Included necessary imports and setup for testing with HTTP service.
- Added new test cases for various validator configurations, including "ManyValidatorPubkeysHoodi" and "MixedIndicesAndPubKeysHoodi". - Introduced a network parameter to conditionally skip tests based on the current network. - Updated the service initialization to use a test service instead of the HTTP service.
- Added test cases for "MixedIndicesAndPubKeysHoodi" and "NegativeRewardsHoodi". - Introduced a network parameter to conditionally skip tests based on the current network. - Updated service initialization to utilize a test service instead of the HTTP service.
- Updated the test to utilize a test service instead of the HTTP service. - Added handling for new block versions: Electra and Fulu, including their respective signed block structures. - Improved test coverage for different block versions in the submit proposal functionality.
- Updated the test to create an Electra attestation with proper committee bits. - Adjusted the versioned attestation structure to accommodate Electra's requirements. - Enhanced error handling to account for potential validation errors during testing.
- Added a new test case for "GoodHoodi" to validate block rewards on the hoodi network. - Introduced a network parameter to conditionally skip tests based on the current network. - Updated service initialization to utilize a test service instead of the HTTP service.
- Added a new test case for "GenesisHoodi" to validate beacon block roots on the hoodi network. - Introduced a network parameter to conditionally skip tests based on the current network. - Updated service initialization to utilize a test service instead of the HTTP service.
- Added a new test case for "GenesisHoodi" to validate beacon block headers on the hoodi network. - Introduced a network parameter to conditionally skip tests based on the current network. - Updated service initialization to utilize a test service instead of the HTTP service.
- Upgraded `github.com/stretchr/testify` from v1.8.4 to v1.11.1 for enhanced testing capabilities. - Refactored multiple HTTP test files to utilize a test service instead of the HTTP service, improving test isolation and reliability. - Introduced network parameters in various tests to conditionally skip based on the current network, enhancing test coverage across different environments. - Added new test cases and improved error handling in several existing tests to ensure robustness and accuracy.
This guarantees a sooner processing of the slot calculated time.
| opts: &api.BeaconStateOpts{}, | ||
| err: "no state specified", | ||
| }, | ||
| // { |
There was a problem hiding this comment.
Did you intend to leave this commented out?
There was a problem hiding this comment.
Yes, that test case is hitting Context Deadline even after 10 minutes. It might be overwhelming for the CL to get that state. How do you advice to proceed?
http/attestationpool.go
Outdated
| return errors.New("attestation data not for requested committee index") | ||
| return errors.New("attestation data not for requested committee index") | ||
| } | ||
| // No check for committee index, so we match all committee indices. |
There was a problem hiding this comment.
I'd reword this to be a bit more explicit - E.g. No commitee_index supplied in opts so skipping check.
A more idiomatic golang way of handling this might be to do this prior to the loop:
if opts.CommitteeIndex == nil {
// No committee index specified in opts so skipping check.
return nil
}
for _, committeeIndex := range data.CommitteeBits.BitIndices() {
if phase0.CommitteeIndex(committeeIndex) == *opts.CommitteeIndex {
// We have a match.
return nil
}
}
return errors.New("attestation data not for requested committee index")For my benefit - what is the usecase where we ask for the attestation pool without specifying a committee index?
There was a problem hiding this comment.
Agree, I refactored the code and commented better the new condition: 7343c7d
There was a problem hiding this comment.
what is the usecase where we ask for the attestation pool without specifying a committee index?
Looking at your comment, I noticed that we might not need to do the filtering anymore.
Electra introduced the committee_index query string for /eth/v2/beacon/pool/attestations. Enabling it tells the CL client to filter the attestations from the attestations pool by that index, therefore we might not need to filter them anymore in go-eth2-client
Anyway, any user of go-eth2-client might never define the CommitteeIndex as nil in the opts, emulating a call to /eth/v2/beacon/pool/attestations without the committee_index query string. I thought we wanted go-eth2-client to support this use case, which would return all the attestations in the attestation pool unfiltered.
Let me know if we should proceed with any of the following options:
- Remove the filter from
verifyElectraAttestation(and let the CL return the data already filtered) - Keep the filter and my change (skip the filter if no committee index was provided)
- Updated the `HashTreeRootWith` method to check for nil blobs and adjusted the length validation to ensure proper handling of blob sizes. - Introduced new test files for blobs in both the `api/v1` and `http` packages, covering various scenarios including successful blob retrieval and error handling. - Enhanced existing blobsidecars tests by replacing direct HTTP service calls with a test service for improved reliability and isolation.
- Updated the `verifyElectraAttestation` function to streamline committee index checks. - Improved error handling to ensure clarity when attestation data does not match the requested committee index.
|
@Bez625 please notice I committed a ssz file in 8fa8b27 to use it as testdata and run the tests deterministically with the same slot data in ssz format in every run. If we don't want to keep this ssz file, I could try to fetch it every time but that will depend on the CL having that slot data and also the point of the test is trying the Blobs' ssz methods with a slot around the present time in Hoodi (Fulu). |
- Added conditional debug logging based on the `HTTP_DEBUG_LOG_ENABLED` environment variable.
- Removed the hardcoded committee index in the "Previous Slot" test case to allow for dynamic committee index checks. - Introduced a new test function, `TestAttestationPoolCommitteeIndexSet`, to validate attestations for various committee indices, improving test coverage. - Added a helper function, `collectCommitteeIndicesWithAttestations`, to gather committee indices with attestations for more robust testing.
…ice initialization within the test suite. - Updated the `initGlobalHTTPService` and `testService` functions to conditionally include the `Authorization` header if the `HTTP_BEARER_TOKEN` environment variable is set, improving flexibility for testing scenarios.
- Introduced a new GitHub Actions workflow to automate HTTP tests on pull requests and manual triggers. - Set up environment variable validation to ensure required variables are defined before running tests.
…rer token authentication - Updated multiple test files to conditionally include the `Authorization` header based on the presence of the `HTTP_BEARER_TOKEN` environment variable, enhancing flexibility in testing scenarios. - Replaced direct service initialization with a `testService` function for improved consistency across tests.
- Updated the test for Sync Committee Rewards to provide more detailed skip messages based on network conditions. - Replaced the existing JSON equality check with a new function that validates committee rewards, improving error handling and clarity in test failures. - Introduced a new type, `committeeReward`, to facilitate structured JSON unmarshalling for reward comparisons.
- Updated the `TestBlobs` function to use a predefined `rawBlobs` variable instead of reading from a file, improving test performance and reliability. - Renamed the variable for clarity from `blob.ssz` to `rawBlobs`. - Removed the `blob.ssz` test data file as it is no longer needed, streamlining the test setup.
This PR significantly enhances the HTTP test suite for go-eth2-client with improved test coverage, network-aware testing, and support for newer Ethereum consensus layer forks (Electra and Fulu). The changes focus on making tests more robust, maintainable, and capable of running against different beacon node implementations and networks.
Key Changes
CI/CD Integration
.github/workflows/http-tests.ymlfor automated HTTP testingHTTP_ADDRESS,HTTP_TEST_CONCURRENCY)HTTP_BEARER_TOKENenvironment variable for authenticating with protected beacon nodesTest Infrastructure Improvements
HTTP_TEST_CONCURRENCYenvironment variable to control parallel test execution (default: sequential)Enhanced Test Coverage
Bug Fixes