Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

chore(starknet_http_server): add metrics tests, still partial #2885

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

chore(starknet_http_server): add metrics tests, still partial #2885

wants to merge 1 commit into from
+94 −4

Conversation

Itay-Tsabary-Starkware
Copy link
Contributor

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware commented Dec 22, 2024 •
edited
Loading

Stack:

⚠️ Part of a stack created by spr. Do not merge manually using the UI - doing so may have unexpected results.

This was referenced Dec 22, 2024
Open
Merged
Merged
@reviewable-StarkWare
Copy link

This change is Reviewable

graphite-app[bot]
graphite-app bot reviewed Dec 22, 2024
View reviewed changes
crates/starknet_http_server/src/http_server_test.rs Outdated
Comment on lines 90 to 91
let added_transactions_total_count =
parse_numeric_metric::<usize>(&metrics, ADDED_TRANSACTIONS_SUCCESS.0);
let added_transactions_success_count =
parse_numeric_metric::<usize>(&metrics, ADDED_TRANSACTIONS_TOTAL.0);
Copy link

Choose a reason for hiding this comment

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

The metric parsing variables appear to be swapped. added_transactions_total_count is parsing ADDED_TRANSACTIONS_SUCCESS when it should parse ADDED_TRANSACTIONS_TOTAL, and added_transactions_success_count is parsing ADDED_TRANSACTIONS_TOTAL when it should parse ADDED_TRANSACTIONS_SUCCESS. This mismatch would cause the test to validate incorrect metric values. To fix this, swap the metric constants in the parse_numeric_metric calls.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

crates/starknet_http_server/src/http_server_test.rs
Comment on lines +58 to +59
i += 1;
match i {
Copy link

Choose a reason for hiding this comment

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

The counter i is incremented before the match statement, which causes the first case (i=0) to be skipped. To properly test all cases, either move the increment after the match or initialize i to -1. This ensures the error case is actually tested.

Spotted by Graphite Reviewer

Is this helpful? React 👍 or 👎 to let us know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@github-actions GitHub Actions
Copy link

github-actions bot commented Dec 22, 2024 •
edited
Loading

Artifacts upload workflows:

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware force-pushed the spr/main/523a1d3e branch 2 times, most recently from 478eed2 to fd45754 Compare December 23, 2024 07:16
@Itay-Tsabary-Starkware Itay-Tsabary-Starkware mentioned this pull request Dec 23, 2024
Open
Itay-Tsabary-Starkware
Itay-Tsabary-Starkware commented Dec 23, 2024
View reviewed changes
Copy link
Contributor Author

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Dismissed @graphite-app[bot] from a discussion.
Reviewable status: 0 of 4 files reviewed, 1 unresolved discussion (waiting on @graphite-app[bot])

crates/starknet_http_server/src/http_server_test.rs
Comment on lines +58 to +59
i += 1;
match i {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

nadin-Starkware
nadin-Starkware requested changes Dec 23, 2024
View reviewed changes
Copy link
Collaborator

@nadin-Starkware nadin-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: 1 of 4 files reviewed, 2 unresolved discussions (waiting on @graphite-app[bot] and @Itay-Tsabary-Starkware)

crates/starknet_http_server/src/http_server_test.rs line 50 at r3 (raw file):

    // The current setup does not test that. Fix the bug and update this test accordingly.
    // Create a mock gateway client that returns arbitrary responses.
    let txs_to_send = 3;

Should this be a constant?

Code quote:

txs_to_send

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware changed the base branch from spr/main/523a1d3e to main December 24, 2024 10:16
@Itay-Tsabary-Starkware Itay-Tsabary-Starkware changed the base branch from main to spr/main/523a1d3e December 24, 2024 10:17
@Itay-Tsabary-Starkware Itay-Tsabary-Starkware mentioned this pull request Dec 24, 2024
Merged
@Itay-Tsabary-Starkware Itay-Tsabary-Starkware changed the base branch from spr/main/523a1d3e to main December 24, 2024 13:35
@Itay-Tsabary-Starkware Itay-Tsabary-Starkware force-pushed the spr/main/ce82f0b7 branch 2 times, most recently from c2c4748 to 745ef6b Compare December 25, 2024 09:12
@github-actions GitHub Actions
Copy link

Benchmark movements:
full_committer_flow performance regressed!
full_committer_flow time: [30.073 ms 30.174 ms 30.294 ms]
change: [+1.5091% +1.9154% +2.3720%] (p = 0.00 < 0.05)
Performance has regressed.
Found 3 outliers among 100 measurements (3.00%)
2 (2.00%) high mild
1 (1.00%) high severe

Itay-Tsabary-Starkware
Itay-Tsabary-Starkware commented Dec 25, 2024
View reviewed changes
Copy link
Contributor Author

@Itay-Tsabary-Starkware Itay-Tsabary-Starkware left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 4 files reviewed, 2 unresolved discussions (waiting on @graphite-app[bot] and @nadin-Starkware)

crates/starknet_http_server/src/http_server_test.rs line 50 at r3 (raw file):

Previously, nadin-Starkware (Nadin Jbara) wrote…

Should this be a constant?

Done.

crates/starknet_http_server/src/http_server_test.rs Outdated
Comment on lines 90 to 91
let added_transactions_total_count =
parse_numeric_metric::<usize>(&metrics, ADDED_TRANSACTIONS_SUCCESS.0);
let added_transactions_success_count =
parse_numeric_metric::<usize>(&metrics, ADDED_TRANSACTIONS_TOTAL.0);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@graphite-app graphite-app[bot] graphite-app[bot] left review comments

@nadin-Starkware nadin-Starkware nadin-Starkware requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Itay-Tsabary-Starkware @reviewable-StarkWare @nadin-Starkware