chore: (PRO-271) Update / cleanup Makefile and add debug commands and test runner

[dev-jodee]

• Simplified .gitignore to exclude all .pid files.
• Added new debug command targets in DEBUG_COMMANDS.makefile for various test scenarios.
• Updated Makefile to include DEBUG_COMMANDS.makefile and added debug-related targets.
• Modified BUILD.makefile to specify the binary name for installation.
• Adjusted CLIENT.makefile to streamline TypeScript client generation process.

chore: Implement rust test runner

• Implemented rust test runner instead of having complex makefiles and obscure code, will be easier to read and control
• Cleaned up a lot of repeated constants for file names and duplicated code that was complex across our testing utils / helpers
• Added a new directory for test accounts in the fixtures.
• Removed unused JSON files for examples
• Refactored test account structure to include additional fields for lookup tables and token accounts.
• Cleaned up imports and adjusted helper functions for better organization.
• Cleaned up a lot the lookup table healper, to make it more dynamic

---

Important

Introduces a comprehensive test runner for Kora, managing integration tests, configurations, and test account setups for Rust and TypeScript environments.

• Test Runner:
  • Introduces test_runner.rs to manage integration tests for Kora.
  • Supports filtering test phases and verbose output.
  • Manages Solana test validator and Kora RPC server lifecycle.
• Configuration:
  • Adds test_cases.toml for defining test phases and configurations.
  • Implements TestRunnerConfig and TestPhaseConfig in config.rs.
• Account Management:
  • Defines AccountFile enum in accounts.rs for managing test account files.
  • Implements functions to set environment variables and download accounts.
• Test Execution:
  • Implements TestCommandHelper in commands.rs to run Rust and TypeScript tests.
  • Supports output filtering and colorization in output.rs.
• Utilities:
  • Adds kora.rs for managing Kora server processes and port allocation.
  • Includes validator.rs for starting Solana test validator with account loading.

^{This description was created by for 8f3b835. You can customize this summary. It will automatically update as commits are pushed.}

---

📊 Unit Test Coverage

Unit Test Coverage: 85.8%

View Detailed Coverage Report

[linear]

PRO-271 Improved Testing Infrastructure

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed everything up to 011a48d in 2 minutes and 20 seconds. Click for details.

• Reviewed 1880 lines of code in 25 files
• Skipped 2 files when reviewing.
• Skipped posting 13 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. Makefile:10

• Draft comment:
  Good that DEBUG.makefile is now included to provide extra debug targets. Make sure this doesn't inadvertently expose sensitive debug output in production builds.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50% None

2. makefiles/BUILD.makefile:1

• Draft comment:
  Removal of the 'build-tk' and 'run-presigned' targets appears intentional. Confirm that no CI or developer workflow still depends on these targets.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50% None

3. makefiles/CLIENT.makefile:1

• Draft comment:
  The 'gen-ts-client' target now depends on 'openapi'. Verify that the openapi spec is consistently generated and valid before client generation.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50% None

4. makefiles/DEBUG.makefile:1

• Draft comment:
  The new debug targets provide a clear set of commands for different test scenarios. Consider adding inline comments or usage instructions so developers know when to use each debug target.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50% None

5. tests/Cargo.toml:7

• Draft comment:
  Renaming the binary from 'setup_test_env' to 'test_runner' simplifies the test runner setup. Ensure that documentation and scripts referencing the old name are updated.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50% None

6. tests/payment_address/payment_address_v0_tests.rs:15

• Draft comment:
  Importing get_fee_for_default_transaction_in_usdc improves consistency for fee calculation across tests.
• Reason this comment was not posted:
  Confidence changes required: 10% <= threshold 50% None

7. tests/rpc/transaction_signing.rs:342

• Draft comment:
  Using expect with a clear error message on get_test_disallowed_address improves debuggability.
• Reason this comment was not posted:
  Confidence changes required: 10% <= threshold 50% None

8. tests/src/bin/test_runner.rs:1

• Draft comment:
  The new Rust test runner consolidates many testing phases and debug operations. Given its complexity, consider refactoring into smaller modules and adding more documentation or logging for error cases.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the file is long, it's well-structured with clear sections and good internal documentation via comments. The suggestion to refactor into modules is reasonable but vague. The error handling seems thorough with detailed error messages. The comment doesn't point to specific areas that need more documentation or specific modules that should be extracted. The comment is too general and doesn't provide specific guidance on what should be refactored or what documentation is missing. It's more of an observation than an actionable suggestion. While code organization could potentially be improved, the current structure is clear and functional. Without specific suggestions for improvement, the comment adds little value. The comment should be deleted as it makes only vague suggestions without specific, actionable recommendations for improvement.

9. tests/src/common/client.rs:10

• Draft comment:
  Refactoring to use environment variable constants (RPC_URL_ENV, TEST_SERVER_URL_ENV) improves maintainability.
• Reason this comment was not posted:
  Confidence changes required: 10% <= threshold 50% None

10. tests/src/common/helpers.rs:13

• Draft comment:
  The fee calculation function is clear and well-commented. This aids in consistency across tests.
• Reason this comment was not posted:
  Confidence changes required: 10% <= threshold 50% None

11. tests/src/common/lookup_tables.rs:40

• Draft comment:
  Transitioning from a static cache to using environment variables for lookup table addresses aids dynamic testing; however, ensure that missing variables result in clear error messages.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50% None

12. tests/src/common/setup.rs:129

• Draft comment:
  The slot waiting logic after setting up accounts (await_for_slot) is effective; consider parameterizing the wait duration to accommodate slower environments.
• Reason this comment was not posted:
  Confidence changes required: 20% <= threshold 50% None

13. tests/src/common/setup.rs:136

• Draft comment:
  Typographical check: The return statement now uses account_infos (plural). Please verify if this is intentional as the previous code used account_info (singular). If this was meant to be a renaming, consider ensuring consistency with the rest of the code.
• Reason this comment was not posted:
  Comment looked like it was already resolved.

Workflow ID: wflow_rE76jBIQJQe0G1zy

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[github-actions]

📊 TypeScript Coverage Report

Coverage: 82.0%

View detailed report

Coverage artifacts have been uploaded to this workflow run.
View Artifacts

[amilz]

@dev-jodee pushed the TS fix to the PR:

• checks if mint already exists to avoid attempting mint when it exists (since your env setup takes care of this)
• update sender address to match your env
• remove turnkey/privy tests b/c we get rate limited w/ those / non-standard & have not been in our CI

[ellipsis-dev]

Caution

Changes requested ❌

Reviewed everything up to 20e9fd0 in 2 minutes and 32 seconds. Click for details.

• Reviewed 4157 lines of code in 52 files
• Skipped 2 files when reviewing.
• Skipped posting 12 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. tests/src/common/setup.rs:61

• Draft comment:
  There are multiple calls to dotenv::dotenv(). Consider centralizing initialization (e.g. in main/test setup) so each module doesn't redundantly call it.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

2. tests/src/common/helpers.rs:23

• Draft comment:
  The parse_private_key_string helper is thin; consider adding documentation or more robust error handling to clarify acceptable formats.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

3. tests/src/test_runner/kora.rs:31

• Draft comment:
  Port management using a Mutex over a HashSet works but might be a bottleneck if many tests run concurrently; consider investigating more scalable approaches if concurrency increases.
• Reason this comment was not posted:
  Confidence changes required: 30% <= threshold 50% None

4. tests/src/test_runner/output.rs:58

• Draft comment:
  ANSI escape sequences are used for coloring output. This may not work on Windows. Consider using a cross-platform crate (e.g. 'ansi_term' or 'colored') or add conditional compilation for Windows.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 30% vs. threshold = 50% This is a test output formatter, so coloring is important for readability. The suggestion to use a cross-platform crate is reasonable and actionable. However, this is test code, not production code. Most developers run tests in Unix-like environments (Linux/Mac/WSL). The current implementation is simple and works for the majority case. I might be underestimating the number of developers who run tests directly on Windows. The comment could help prevent future issues if Windows testing becomes important. While Windows compatibility could be nice, the current implementation is simple and sufficient for test output. The cost of adding a dependency just for test coloring may not be worth it. The comment should be removed. While technically correct, the suggestion is overkill for test output formatting where Unix-like environments are common and the current solution is simple and adequate.

5. tests/src/test_runner/validator.rs:59

• Draft comment:
  check_test_validator uses DEFAULT_RPC_URL. If a custom RPC URL is provided, this may not perform as intended. Consider parameterizing the URL.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a test validator setup file. The DEFAULT_RPC_URL is likely intentionally hardcoded since this is specifically for local test validation. The check_test_validator function takes a parameter probably for reuse elsewhere, but in this context, we always want to check the default test validator URL. Making this parameterizable would add unnecessary complexity. The comment raises a valid point about API design consistency. Maybe there are use cases where someone would want to start a test validator on a different port. While technically correct, this is test infrastructure code where simplicity and predictability are more valuable than flexibility. The DEFAULT_RPC_URL is appropriate for a test validator. The comment should be deleted as it suggests adding unnecessary complexity to test infrastructure code where using a default URL is appropriate.

6. tests/src/test_runner/commands.rs:90

• Draft comment:
  Test commands for TypeScript tests use 'pnpm run' with current_dir set. Ensure that the working directory 'sdks/ts' exists in all environments to avoid path issues.
• Reason this comment was not posted:
  Confidence changes required: 50% <= threshold 50% None

7. tests/src/common/lookup_tables.rs:116

• Draft comment:
  The loop waiting for lookup table activation might potentially wait indefinitely if the condition is never met. Consider adding a timeout to avoid infinite waiting.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The loop is waiting for Solana slots to advance, which happens every ~400ms in normal operation. The sleep duration matches this timing. Since this is test code, a timeout would be good practice, but slots will reliably advance in a working test environment. The current implementation is reasonable for test code. The comment raises a valid point about defensive programming. In theory, if the node is stuck or slots stop advancing, this could hang indefinitely. However, this is test code where a timeout would just mask deeper problems. If slots aren't advancing, the test environment is broken and should fail fast rather than timeout. The comment should be removed. While technically correct, adding a timeout here would not improve the test code and could hide more serious test environment issues.

8. crates/lib/src/validator/config_validator.rs:1052

• Draft comment:
  It looks like the assert statement on this line is missing a closing parenthesis (and possibly a semicolon) at the end. Please verify that the intended syntax is correct.
• Reason this comment was not posted:
  Comment was not on a location in the diff, so it can't be submitted as a review comment.

9. tests/src/common/helpers.rs:12

• Draft comment:
  Typographical suggestion: In the comment "This is used for a lot of tests that only has sender and fee payer as signers", consider changing "has" to "have" for proper subject-verb agreement.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While the grammar correction is technically correct, it's a very minor issue in a test file's documentation. The meaning is perfectly clear either way. According to the rules, we should not make purely informative comments or comments about obvious/unimportant issues. Grammar in test file comments is not critical to code functionality. The grammar error could theoretically make the code documentation less professional. Documentation quality does matter for maintainability. While documentation quality matters, this is a minor grammar issue in a test helper file that doesn't impact understanding. The rules specifically say not to make unimportant comments. This comment should be deleted as it's too minor of an issue to warrant a PR comment, especially in test code documentation.

10. tests/src/common/lookup_tables.rs:51

• Draft comment:
  Typographical issue: The error message refers to "TEST_DISALLOWED_LOOKUP_TABLE_ADDRESS environment variable" while the variable being read is named "TEST_DISALLOWED_LOOKUP_TABLE_ADDRESS_ENV". Consider updating the error message for consistency.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% This is a minor inconsistency in error message text. While technically correct, error messages are part of the developer experience. However, this seems like a very minor issue that doesn't significantly impact code functionality or maintainability. The error message is still clear enough to understand what's missing. The inconsistency could potentially cause confusion for developers trying to set up the environment. However, most developers would quickly figure out the correct variable name from the code. The impact of this inconsistency is minimal. Developers can easily see the correct variable name in the code, and the error message still communicates the core issue. This comment, while technically correct, points out an extremely minor issue that doesn't warrant a code change. The error message is still functional and clear enough.

11. tests/src/test_runner/test_cases.toml:21

• Draft comment:
  Typo noticed: The property name "initialize_payments_atas" seems to contain a misspelling. Please verify if it should be "initialize_payments_data" or another intended term.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% ATAS could be an acronym or domain-specific term, especially since this is in a test configuration file. Without more context about what ATAS means in this codebase, we can't assume it's a typo. The comment is speculative and asks for verification rather than pointing out a clear issue. Maybe ATAS is actually a typo and the comment is helping catch a real issue. The similar word "data" does make some logical sense in this context. Even if it might be a typo, we don't have strong evidence that it is. The comment violates our rules by asking for verification and being speculative. Delete the comment because it's speculative and asks for verification without strong evidence of an actual issue.

12. tests/src/test_runner/validator.rs:30

• Draft comment:
  The println! macro on this line appears to be attempting string interpolation with curly braces, but in Rust you should use a placeholder and pass the variable as an argument. For example, change: println!("⚠️ Transfer hook program not found at: {TRANSFER_HOOK_PROGRAM_PATH}"); to: println!("⚠️ Transfer hook program not found at: {}", TRANSFER_HOOK_PROGRAM_PATH); This will correctly display the value of TRANSFER_HOOK_PROGRAM_PATH.
• Reason this comment was not posted:
  Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 0% vs. threshold = 50% The comment is technically incorrect. Since Rust 1.58.0, string literals can directly interpolate constants and statics using braces without needing the placeholder syntax. The current code is actually using valid modern Rust syntax. The suggested change would work but is unnecessary and actually less ergonomic than the current code. Could there be a minimum Rust version requirement for the project that requires supporting older versions where this syntax wasn't available? Even if there was a minimum version requirement, that would be a project-wide decision that should be documented elsewhere, not enforced through PR comments. The current syntax is the preferred modern way. The comment should be deleted as it suggests changing valid, modern Rust syntax to a more verbose older style without any benefit.

Workflow ID: wflow_2StSC1uOdxLqPicO

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}

[amilz]

🔥

[ellipsis-dev]

Important

Looks good to me! 👍

Reviewed 8f3b835 in 1 minute and 20 seconds. Click for details.

• Reviewed 69 lines of code in 4 files
• Skipped 0 files when reviewing.
• Skipped posting 4 draft comments. View those below.
• Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.

1. Makefile:16

• Draft comment:
  Ensure the new ordering in the 'test-all' target (test, test-ts, test-integration) is intentional and does not affect dependencies.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

2. tests/src/bin/test_runner.rs:419

• Draft comment:
  Added leading newlines in the success and failure messages for better output formatting. Verify that these extra newlines are consistent across all phases.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

3. tests/src/test_runner/commands.rs:90

• Draft comment:
  Removal of the ENABLE_AUTH env variable setting for 'typescript_auth' tests—ensure this change is safe and that auth tests are configured elsewhere if needed.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

4. tests/src/test_runner/output.rs:176

• Draft comment:
  Simplified the filter_and_colorize_output function by always applying colorize. Confirm this behavior is correct when the filtered output is empty.
• Reason this comment was not posted:
  Confidence changes required: 0% <= threshold 50% None

Workflow ID: wflow_eHJYONiDr5Efz590

^{You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.}