test: fix and rework e2e and improve code coverage #2029
Conversation
WalkthroughReworks E2E test harness to use CLI-backed contexts and Exec-style clitestutil helpers, narrows integration test selection to ./tests/e2e, updates gas flag helper usage, adds many unit tests and test data, changes network API host to 127.0.0.1, and adds small e2e helpers and selective test skips in certs tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)tests/e2e/deployment_cli_test.go (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
🔇 Additional comments (4)
Comment |
cloud-j-luna
force-pushed
the
test/increase-coverage
branch
from
33acd63 to
def8e93
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (5)
x/market/query/path_test.go (1)
196-279: Tighten error-type checks in TestParseOrderPath
TestParseOrderPathcarries anerrTypefield, but the current logic only checkserr != niland theerrTypecomparison block is effectively unreachable, so you never actually verify that specific cases returnErrInvalidPath.Consider simplifying to an explicit
errors.Ischeck, e.g.:if tt.wantErr { if err == nil { t.Fatal("parseOrderPath() expected error, got nil") } if tt.errType != nil && !errors.Is(err, tt.errType) { t.Fatalf("parseOrderPath() error = %v, want %v", err, tt.errType) } return }(with
errorsimported from the standard library). This would make the intent oferrTypeconcrete and remove the dead comparison block.x/take/keeper/keeper_test.go (4)
20-42: Keeper test harness is solid; consider DRYing constantsThe in‑memory keeper/context setup looks correct and idiomatic for Cosmos SDK tests. To reduce drift, you might factor the
"take"store key and authority address into package‑levelconsts and reuse them here and in the assertions below, instead of repeating the literals.
75-86: Authority test is correct; consider centralizing the addressThe test correctly asserts that
GetAuthority()is non‑empty and equals the expected address. Since the same bech32 string is also used insetupKeeper, consider extracting it into a sharedconstused in both places to avoid future mismatch if the test authority ever changes.
88-217: Param round‑trip and validation coverage is good; subtest isolation is optionalNice table‑driven coverage across valid/invalid params (bounds, missing
uakt, duplicate denoms, etc.) and round‑trip viaGetParams. One optional improvement would be to callsetupKeeper(t)inside eacht.Runto fully isolate state between cases, especially around the erroringSetParamspaths, but given current assertions this shared keeper/context is acceptable.
334-379:findRatetests cover key branches; optional extra for unknown denomThe cases for per‑denom overrides and default fallback via empty denom look solid. If you want to exercise the “unknown non‑empty denom uses default rate” branch explicitly (complementing
TestSubtractFees’s"atom"case), you could add adenom: "atom"entry here as well, but that’s purely optional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
make/test-integration.mk(2 hunks)tests/e2e/certs_cli_test.go(12 hunks)tests/e2e/certs_grpc_test.go(2 hunks)tests/e2e/cli_test.go(1 hunks)tests/e2e/deployment_cli_test.go(13 hunks)tests/e2e/deployment_grpc_test.go(2 hunks)tests/e2e/helpers.go(1 hunks)tests/e2e/market_cli_test.go(27 hunks)tests/e2e/market_grpc_test.go(17 hunks)tests/e2e/provider_cli_test.go(3 hunks)tests/e2e/provider_grpc_test.go(3 hunks)testutil/network/network.go(1 hunks)testutil/network_suite.go(5 hunks)x/deployment/query/path_test.go(1 hunks)x/deployment/query/types_test.go(1 hunks)x/deployment/testdata/deployment-multi-groups.yaml(1 hunks)x/escrow/client/util/util_test.go(1 hunks)x/market/query/path_test.go(1 hunks)x/market/query/types_test.go(1 hunks)x/provider/query/types_test.go(1 hunks)x/take/keeper/keeper_test.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (10)
x/escrow/client/util/util_test.go (1)
x/escrow/client/util/util.go (2)
LeaseCalcBalanceRemain(9-11)LeaseCalcBlocksRemain(13-15)
x/deployment/query/path_test.go (2)
cmd/akash/cmd/testnetify/config.go (1)
AccAddress(32-34)x/deployment/query/path.go (1)
ParseGroupPath(38-54)
tests/e2e/market_cli_test.go (3)
tests/e2e/cli_test.go (1)
DefaultDeposit(15-15)testutil/network/network.go (1)
Network(126-132)x/market/query/types.go (2)
Bid(17-17)Lease(22-22)
x/market/query/types_test.go (1)
x/market/query/types.go (7)
Order(12-12)OrderFilters(32-38)Bid(17-17)BidFilters(41-47)Lease(22-22)LeaseFilters(50-56)Orders(14-14)
x/market/query/path_test.go (2)
x/market/query/types.go (3)
OrderFilters(32-38)BidFilters(41-47)LeaseFilters(50-56)x/market/query/path.go (3)
OrderPath(35-37)LeasePath(55-57)ParseLeasePath(105-112)
x/take/keeper/keeper_test.go (1)
x/escrow/keeper/querier.go (1)
NewQuerier(7-9)
x/deployment/query/types_test.go (2)
cmd/akash/cmd/testnetify/config.go (1)
AccAddress(32-34)x/deployment/query/types.go (3)
Deployment(35-38)DeploymentFilters(14-20)Deployments(51-51)
testutil/network_suite.go (1)
testutil/network/network.go (2)
Validator(137-159)Config(91-114)
tests/e2e/provider_grpc_test.go (1)
testutil/network/network.go (1)
Network(126-132)
tests/e2e/deployment_grpc_test.go (2)
testutil/network/network.go (1)
Network(126-132)tests/e2e/cli_test.go (1)
DefaultDeposit(15-15)
🪛 GitHub Check: lint-go
x/deployment/query/path_test.go
[failure] 150-150:
File is not properly formatted (gofmt)
x/market/query/types_test.go
[failure] 380-380:
File is not properly formatted (gofmt)
x/take/keeper/keeper_test.go
[failure] 70-70:
QF1008: could remove embedded field "Keeper" from selector (staticcheck)
x/deployment/query/types_test.go
[failure] 206-206:
File is not properly formatted (gofmt)
🪛 golangci-lint (2.5.0)
x/deployment/query/path_test.go
[error] 150-150: File is not properly formatted
(gofmt)
x/market/query/types_test.go
[error] 380-380: File is not properly formatted
(gofmt)
x/take/keeper/keeper_test.go
[error] 70-70: QF1008: could remove embedded field "Keeper" from selector
(staticcheck)
x/deployment/query/types_test.go
[error] 206-206: File is not properly formatted
(gofmt)
🔇 Additional comments (50)
x/deployment/testdata/deployment-multi-groups.yaml (1)
1-62: YAML syntax and structure are well-formed.Overall structure is valid with proper indentation, section organization (services, profiles, deployment), and field definitions. Services expose ports with routing rules, compute profiles specify resource requirements, and placement profiles define regional pricing—all aligning with a comprehensive multi-group deployment test scenario.
x/escrow/client/util/util_test.go (3)
10-129: LGTM! Comprehensive test coverage.The test cases thoroughly cover normal operations, edge cases (zero blocks elapsed, exact depletion, overdraft), and boundary conditions (zero balance, zero lease price, large numbers). The epsilon-based floating-point comparison is appropriate.
272-274: LGTM! Standard floating-point comparison helper.The epsilon-based comparison is the correct approach for floating-point equality testing.
246-257: This test case documents expected behavior; no action needed.The "negative blocks elapsed" scenario is intentionally tested as an extreme value case. The function correctly calculates balance changes when
currBlock < settledAt, resulting in a balance increase (line 254 validatesresult > 1000.0). This is a valid mathematical outcome and the implementation has no semantic constraints. The test appropriately validates that the calculation is consistent in this edge case.tests/e2e/certs_grpc_test.go (1)
24-24: CLI context + gas helper usage looks consistentUsing
s.CLIContext()and switching toWithGasAuto()aligns with the CLI-centric patterns introduced elsewhere in the e2e suite; the flow of the test remains the same.Also applies to: 39-47
tests/e2e/cli_test.go (1)
8-9: DefaultDeposit construction with sdkmath.NewInt looks correctUsing
sdkmath.NewInt(5000000)for thesdk.NewCoinamount matches thecosmossdk.io/math–based integer model used by newer Cosmos SDK versions and should be a no-op behaviourally.Please double-check against the Cosmos SDK version in go.mod that
sdk.NewCoinindeed takes amath.Intand thatsdkmath.NewIntis the recommended constructor in that version.Also applies to: 15-15
testutil/network/network.go (1)
297-304: Localhost-only API client URL is reasonableBinding the API to
tcp://0.0.0.0:<port>but constructing the client URL ashttp://127.0.0.1:<port>is a sensible tightening: external reachability is unchanged, while tests consistently talk to loopback.x/market/query/types_test.go (1)
11-331: Filter Accept() coverage is solidThe table-driven tests for
OrderFilters.Accept,BidFilters.Accept, andLeaseFilters.Acceptcover the key combinations of owner match/non-match and valid/invalid state, including the “invalid state → owner-only filter” behaviour. This is good targeted coverage for the existing filter logic.x/provider/query/types_test.go (1)
10-108: Provider String() and Address() tests look well-targetedThese tests sensibly cover:
- String representations for single/multiple/empty providers, including separator handling.
Provider.Address()happy path and the panic behaviour on invalid owner.Nice focused coverage on the query types.
x/market/query/path_test.go (1)
11-195: Comprehensive coverage of market path builders and parsersThe tests for
getOrdersPath/getBidsPath/getLeasesPath,OrderPath,getBidPath,LeasePath,orderParts,parseBidPath, andParseLeasePathexercise both happy paths and a good spread of invalid inputs (missing parts, bad numbers, invalid addresses). This is strong, realistic coverage for the path utilities.Also applies to: 281-419
tests/e2e/certs_cli_test.go (1)
21-27: Skips and gas helper updates are sensible; track re-enabling server tests
- Swapping
WithGasAutoFlags()forWithGasAuto()across the certs CLI tests keeps the flag construction centralized in the CLI helper and matches the updated pattern used elsewhere in the suite.- Marking the server-side tests as skipped with clear messages (“TxRevokeServerExec calls wrong command”, “argument requirements may have changed”) is an acceptable temporary workaround, but it does drop coverage; it would be good to open a follow-up issue or TODO to re-enable these once the underlying CLI/testutil behaviour is fixed.
Please confirm in the
pkg.akt.dev/go/clihelpers thatWithGasAuto()is the intended replacement forWithGasAutoFlags()(same or stricter semantics), and consider tracking the skipped server tests in an issue so they don’t remain disabled indefinitely.Also applies to: 29-41, 43-174
make/test-integration.mk (1)
4-4: LGTM!The narrowing of integration test scope to E2E modules with the
e2e.integrationtag is well-aligned with the PR objectives. This provides better test organization and execution control.Also applies to: 24-24
tests/e2e/helpers.go (1)
1-19: LGTM!The
ExecGroupClosehelper provides a clean CLI-centric interface for closing deployment groups in E2E tests, consistent with the broader migration to Exec*-style utilities across the test suite.x/deployment/query/path_test.go (1)
1-149: LGTM! Comprehensive test coverage.The unit tests provide excellent coverage for
ParseGroupPath, including valid paths, invalid inputs, boundary values, and edge cases. The test logic correctly validates parsing behavior.tests/e2e/provider_cli_test.go (2)
30-30: LGTM! CLI-centric context migration.The migration from
context.Background()tos.CLIContext()and the adoption ofExecTxCreateProviderwith flag-based inputs (includingWithGasAuto()) aligns well with the new CLI-centric test infrastructure.Also applies to: 33-43
47-58: LGTM! Consistent API surface updates.The migration to
ExecQueryProviders,ExecQueryProvider, andExecTxUpdateProvideris consistent across all provider-related operations, with proper flag-based inputs and CLI context usage.Also applies to: 64-76, 79-90, 93-106
testutil/network_suite.go (3)
90-101: LGTM! Proper CLI context initialization.The setup correctly initializes the CLI context with address codecs and creates an Akash RPC client. This provides the foundation for CLI-centric test execution across the E2E suite.
115-130: LGTM! CLI-based wallet funding.The migration from direct bank MsgSend broadcasts to
ExecSendaligns with the CLI-centric approach and ensures consistent test execution patterns.
134-141: LGTM! New context accessors.The
CLIContext()andCLIClientContext()accessors provide clean interfaces for tests to access CLI-configured contexts. The updatedClientContext()method correctly usescliCctxas the base context.Also applies to: 202-205
tests/e2e/deployment_grpc_test.go (2)
31-31: LGTM! CLI context adoption.The migration to
s.CLIClientContext()ands.CLIContext()aligns with the new CLI-centric test infrastructure.Also applies to: 36-36
62-74: LGTM! Deployment API surface updates.The migration to
ExecDeploymentCreateandExecQueryDeploymentswith flag-based inputs (With(deploymentPath),WithGasAuto()) is consistent with the broader E2E test refactoring.Also applies to: 77-82
tests/e2e/provider_grpc_test.go (3)
27-28: LGTM! API readiness wait.The use of
WaitForBlocks(2)to ensure API server readiness before running tests is a pragmatic approach for integration test environments.Also applies to: 51-51
33-49: LGTM! Provider test migration.The migration to
ExecTxCreateProviderandExecQueryProviderswith CLI contexts (s.CLIContext(),s.CLIClientContext()) and flag-based inputs is consistent with the CLI-centric test infrastructure.Also applies to: 54-69
74-74: LGTM! REST API test context.Using
s.CLIClientContext()for REST API tests ensures proper codec configuration for JSON unmarshaling.Also applies to: 118-118
tests/e2e/market_cli_test.go (4)
37-37: LGTM! CLI context migration.The consistent use of
s.CLIContext()ands.CLIClientContext()across setup and test methods aligns with the CLI-centric test infrastructure.Also applies to: 47-47, 122-122
71-71: LGTM! Gas flag standardization.The migration from
WithGasAutoFlags()toWithGasAuto()is consistent across all transaction executions.Also applies to: 87-87, 109-109, 134-134, 241-241, 288-288, 371-371, 423-423
126-138: LGTM! Market operation API updates.The migration to Exec* functions (
ExecDeploymentCreate,ExecQueryDeployments,ExecQueryOrders,ExecTxCreateProvider,ExecQueryProviders,ExecCreateBid) with flag-based inputs is consistent with the CLI-centric approach.Also applies to: 141-146, 155-160, 234-245, 248-254, 279-292
285-286: LGTM! Proper numeric value construction.The use of
sdkmath.NewInt()andsdkmath.LegacyMustNewDecFromStr()for constructing coin and decimal values is correct and aligns with the Cosmos SDK math package usage.tests/e2e/deployment_cli_test.go (7)
23-31: LGTM!The struct fields are well-organized with clear naming for the CLI context and key/address pairs for funder and deployer accounts.
33-120: LGTM!The setup correctly initializes keys, funds both accounts with sufficient tokens, and publishes client certificates. The migration to
CLIClientContext()andCLIContext()is consistent, andWithGasAuto()is properly applied to all transactions.
143-189: LGTM!The query tests properly filter by owner to isolate from other tests, and the assertions correctly validate deployment creation and querying with various filters.
241-317: LGTM!The test correctly creates a deployment, retrieves the latest one using the slice index, closes a group, and verifies the group state transitions to
GroupClosed.
369-445: LGTM!The sequential group closing logic correctly verifies that closing one group doesn't affect others, with proper state assertions after each operation.
447-462: LGTM!Good verification that the deployment remains active even after all groups are closed, confirming the expected lifecycle behavior.
319-362: Good addition of multi-group deployment test.The test comprehensively covers the multi-group deployment scenario, verifying initial states and group independence. The referenced testdata file
deployment-multi-groups.yamlexists inx/deployment/testdata/and is properly accessible via the relative path.tests/e2e/market_grpc_test.go (8)
25-32: LGTM!The struct properly stores the client context and market entities (order, bid, lease) for use across test methods.
34-209: LGTM!The setup comprehensively initializes the market test environment: creates certificates, deployments, providers, bids, and leases. The migration to
CLIClientContext()/CLIContext()andWithGasAuto()is consistent throughout.
211-276: LGTM!Well-structured table-driven tests covering various filter combinations for the orders endpoint.
278-342: LGTM!Good coverage of edge cases including empty input, invalid input, and not found scenarios for the order endpoint.
344-415: LGTM!Consistent table-driven test structure for the bids endpoint with appropriate filter combinations.
417-487: LGTM!Proper edge case handling for the individual bid endpoint.
489-560: LGTM!Consistent test coverage for the leases list endpoint with appropriate filter combinations.
562-632: LGTM!Proper edge case handling for the individual lease endpoint, consistent with the overall test patterns in the file.
x/take/keeper/keeper_test.go (7)
44-51: Codec smoke test is appropriateSimple non‑nil assertion on
keeper.Codec()is fine as a smoke test for wiring; no issues from my side.
53-64: StoreKey test is clear and sufficientChecking both non‑nil and that
StoreKey().Name()equals"take"is exactly what you want here; looks good.
219-231: Empty‑storeGetParamsbehaviour is explicitly capturedVerifying that an empty store yields zero values and no denom rates is clear and matches the expectations encoded in the test name; no issues here.
233-332:SubtractFeestests are thorough and well‑balancedGood coverage of happy‑paths and edge cases: small/zero amounts, rounding behaviour, per‑denom overrides vs default rate, and a sanity check that earning+fee always equals the original amount. This should give strong confidence in the fee logic.
381-390: No‑paramsfindRatebehaviour is clearly definedAsserting that
findRatereturns zero when no params have ever been set makes the default behaviour explicit and test‑documented; looks good.
392-420: Zero‑rateSubtractFeescase is correctThe test correctly verifies that with 0% rates, the full amount is retained as earnings and the fee is zero, with no unexpected errors. This nicely complements the other
SubtractFeescoverage.
422-450: 100%‑rateSubtractFeesedge case is well‑specifiedChecking that a 100% rate yields zero earnings and the full amount as fees (and no error) cleanly captures the upper‑bound behaviour; no further comments.
cloud-j-luna
changed the title
Description
Closes: #XXXX
Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow-up issues.
I have...
!to the type prefix if API or client breaking changeCHANGELOG.md