Skip to content

implement cli with tests#161

Merged
gballet merged 42 commits intoblockblaz:mainfrom
bomanaps:test/cli-sim-endpoints-conditions
Sep 22, 2025
Merged

implement cli with tests#161
gballet merged 42 commits intoblockblaz:mainfrom
bomanaps:test/cli-sim-endpoints-conditions

Conversation

@bomanaps
Copy link
Copy Markdown
Contributor

@bomanaps bomanaps commented Sep 10, 2025

I successfully built the new sim command and added a set of very fast unit tests for it. These tests confirm that the command correctly understands its arguments, like port numbers and network settings, and they run in under a second.

  1. What's Missing

We are missing tests that check if the live web server started by the sim command is actually working. Our current tests don't make real HTTP requests to the /metrics and /health endpoints to verify they are online and responding correctly.

  1. Reason Why

This gap exists because of a trade-off between speed and completeness. To properly test the web server, we would need to start the entire blockchain simulation, which is very slow.

In short, the command itself is fully built, but we don't have automated tests to prove its web server component works because those tests are too slow to run regularly.

closes #154

@bomanaps
Copy link
Copy Markdown
Contributor Author

@gballet @g11tech

@bomanaps
Copy link
Copy Markdown
Contributor Author

@noopur23

@gballet
Copy link
Copy Markdown
Contributor

gballet commented Sep 11, 2025

@gballet @g11tech

you know that you can assign us for review right? or is that option not available to you for some reason?

@gballet gballet requested review from g11tech and gballet September 11, 2025 07:52
@bomanaps bomanaps requested a review from g11tech September 12, 2025 18:09
@bomanaps
Copy link
Copy Markdown
Contributor Author

@g11tech

}

test "CLI sim command argument validation" {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove extra \n

try std.testing.expect(std.mem.eql(u8, startup_steps[3], "create_beam_node"));
try std.testing.expect(std.mem.eql(u8, startup_steps[4], "run_node"));
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i am not sure what you are testing in this entire file, you assign something and then you test the assignment

try beam_node_2.run();
try clock.run();
},
.sim => |simcmd| {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we don't need this command, current beam command is already a 2 (out of 3 node) sim

point of this entire PR was to actually start the sim, actually test that the node is up, actually make an http request to metrics endpoint and see if we can get the metrics etc

@bomanaps bomanaps requested a review from g11tech September 14, 2025 12:48
@bomanaps bomanaps requested a review from g11tech September 18, 2025 14:11
@bomanaps
Copy link
Copy Markdown
Contributor Author

Hello @g11tech I have made the changes

@bomanaps
Copy link
Copy Markdown
Contributor Author

@g11tech

Copy link
Copy Markdown
Contributor

@gballet gballet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am running into an issue where the tests will hang the second time, because the connection is refused. I have been trying to understand what goes wrong, but haven't found out so far. Anyhow, I don't think we can merge this until this is fixed.

build.zig Outdated
const install_cli = b.addInstallArtifact(cli_exe, .{});

// Create simtest step that runs all tests (unit + integration)
const simtests = b.step("simtest", "Run all tests including integration tests");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is not what should happen: there should be simtests, that are the integration tests, and then the tests, which are their separate executable. No need to bundle them together.

Comment on lines +81 to +83
if (retry_count % 20 == 0) {
std.debug.print("DEBUG: Connection attempt {} failed: {}\n", .{ retry_count, err });
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a problem: if it can't connect, then it will spin forever (which happens)

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

max_wait_time is what will break the loop I think (!20 secs)

@bomanaps
Copy link
Copy Markdown
Contributor Author

I am running into an issue where the tests will hang the second time, because the connection is refused. I have been trying to understand what goes wrong, but haven't found out so far. Anyhow, I don't think we can merge this until this is fixed.

I have addressed this not sure if my solution is the best walk around for it but i had to use SO_REUSEADDR the server form the first run was still in use thats why this happens

@bomanaps bomanaps requested a review from g11tech September 21, 2025 16:43
@bomanaps bomanaps requested a review from g11tech September 21, 2025 18:08
@bomanaps
Copy link
Copy Markdown
Contributor Author

@g11tech

@gballet gballet force-pushed the test/cli-sim-endpoints-conditions branch from e9b0a9b to e6251cb Compare September 22, 2025 20:12
gballet
gballet previously approved these changes Sep 22, 2025
runs-on: ${{ matrix.os }}
strategy:
matrix:
os: [ubuntu-latest, macos-latest]
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
os: [ubuntu-latest, macos-latest]
os: [ubuntu-latest, macos-latest, windows-latest]

@gballet gballet merged commit 54e549e into blockblaz:main Sep 22, 2025
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add tests for starting cli and checking various end points

4 participants