Merged
Conversation
The 26 e2e tests are all marked #[ignore] (needed to prevent them from running during `cargo test --all-features` in ci.yml where no localnet is available), but the e2e workflow and local script were missing the `-- --ignored` flag, so the tests were compiled but silently skipped. Also improves the e2e workflow robustness: - Capture localnet logs to a file for debugging - Print logs on failure (similar to aptos-ts-sdk pattern) - Fail explicitly if localnet or faucet don't start in time - Clean up commented-out spec tests section
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where 26 e2e tests were being compiled but never executed in CI due to missing the -- --ignored flag. The tests are intentionally marked with #[ignore] to prevent them from running during regular cargo test (where no localnet is available), but the e2e workflow and local script needed to explicitly opt-in with the -- --ignored flag.
Changes:
- Added
-- --ignoredflag to e2e workflow and local test script to actually execute the ignored tests - Enhanced e2e workflow with better error handling: explicit timeouts, log capture to file, and failure debugging
- Removed commented-out spec-tests-network job that was no longer needed
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
.github/workflows/e2e.yml |
Added -- --ignored flag to cargo test command, improved localnet startup with timeout checks and log capture, enhanced cleanup and debugging |
scripts/run-e2e.sh |
Added -- --ignored flag to cargo test commands for both default and filtered test execution |
Comments suppressed due to low confidence (1)
scripts/run-e2e.sh:139
- The
run_testsfunction builds a shell command string containing untrustedTEST_FILTERdata and executes it viaeval, which allows shell command injection if an attacker can control the script argument. An input like"some_test; rm -rf /"would cause arbitrary commands to run with the privileges of the CI runner or developer invoking this script. To fix this, avoid usingevaland instead invokecargo testdirectly with properly quoted arguments (for example by using a Bash array so that the filter is passed as a single argument rather than interpolated into a command string).
if [[ -n "$TEST_FILTER" ]]; then
test_cmd="cargo test -p aptos-sdk --features 'e2e,full' -- --ignored $TEST_FILTER"
fi
echo "Running: $test_cmd"
eval "$test_cmd"
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The Aptos client was using chain_id 0 for custom network configurations (including when APTOS_LOCAL_NODE_URL is set), causing BAD_CHAIN_ID errors on all transaction submissions against localnet. Add lazy chain_id resolution: - Aptos struct now stores chain_id in RwLock, initialized from config - ensure_chain_id() fetches from node when chain_id is unknown (0) - build_transaction() resolves chain_id in parallel with seq num and gas - ledger_info() updates chain_id as a side effect - BatchOperations also resolves chain_id for custom networks Fix e2e tests that manually build transactions to use ensure_chain_id() instead of hardcoded chain_id values.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
#[ignore]but the CI workflow and local script were missing-- --ignored, so tests were compiled but silently skipped every runscripts/run-e2e.sh): same-- --ignoredfix, test filtering still worksContext
The
#[ignore]attribute on e2e tests is intentional — it prevents them from accidentally running duringcargo test --all-featuresinci.yml(where no localnet is available). Thee2efeature flag controls compilation, while#[ignore]controls runtime execution. The e2e workflow needs-- --ignoredto opt in.Test plan
ci.ymlwith--all-featuresshould still skip e2e tests (no localnet)./scripts/run-e2e.shruns tests locally with-- --ignored./scripts/run-e2e.sh accountstill filters correctly