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.

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

feat: remove jsonrpc client in favor of grpc client #578

Closed
wants to merge 19 commits into from

Conversation

pedrohba1
Copy link
Contributor

No description provided.

@pedrohba1 pedrohba1 requested a review from gusinacio January 16, 2025 05:14
crates/tap-agent/src/agent/sender_account.rs Outdated Show resolved Hide resolved
crates/tap-agent/src/agent/sender_allocation.rs Outdated Show resolved Hide resolved
@gusinacio
Copy link
Member

One test is running forever, could you check if you are properly aborting the task?

@pedrohba1 pedrohba1 self-assigned this Jan 20, 2025
clippy:
- name: Start mock server
run: |
cargo run --bin mock_tap_aggregator_server_runner &
Copy link
Contributor Author

@pedrohba1 pedrohba1 Jan 22, 2025

Choose a reason for hiding this comment

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

So, I've added this so there is less changes and boilerplate code when testing. Otherwise, a lot of tests would require running the mocked gRPC server and shutting it down in the test function. This is because unlike
HttpClientBuilder::defaultthat was being used before,TapAggregatorClientdoes not have a builder and just usesconnect()`. And this fails the tests because there was no server in the dummy endpoint being used.

The other option besides this, is run the Mock in each test that needs it, and shut it down. Similarly to what is done with sender_account.stop_and_wait

Since there are many tests that require this, I think this approach is simpler

@pedrohba1
Copy link
Contributor Author

The test that hangs is in agent::sender_allocation::tests::test_close_allocation_with_pending_fees, not sure why yet. But it does hang right in the sender_allocation.stop_and_wait(None, None).await.unwrap();

@pedrohba1 pedrohba1 requested a review from gusinacio January 22, 2025 05:42
justfile Outdated Show resolved Hide resolved
@@ -1256,10 +1258,6 @@ pub mod tests {
message_receiver.recv().await.unwrap(),
SenderAccountMessage::UpdateReceiptFees(_, ReceiptFees::RavRequestResponse(_))
));

Copy link
Member

Choose a reason for hiding this comment

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

You need to do handle.abort() here, otherwise the test is gonna run forever.

.github/workflows/tests.yml Outdated Show resolved Hide resolved
crates/tap-agent/src/agent/sender_account.rs Outdated Show resolved Hide resolved
crates/tap-agent/src/agent/sender_accounts_manager.rs Outdated Show resolved Hide resolved
@gusinacio
Copy link
Member

gusinacio commented Jan 22, 2025

The test that hangs is in agent::sender_allocation::tests::test_close_allocation_with_pending_fees, not sure why yet. But it does hang right in the sender_allocation.stop_and_wait(None, None).await.unwrap();

When you close an allocation, it tries to create a last_rav by aggregating all receipts until they are all aggregated.

It looks like when you call stop_and_wait it's hanging trying to aggregate receipts, you need to provide a real aggregator for that test, probably.

We were previously mocking the response for the aggregator server, but since we cannot mock HTTP responses with gRPC you need to fix the test to use our new aggregator.

@pedrohba1
Copy link
Contributor Author

The test that hangs is in agent::sender_allocation::tests::test_close_allocation_with_pending_fees, not sure why yet. But it does hang right in the sender_allocation.stop_and_wait(None, None).await.unwrap();

When you close an allocation, it tries to create a last_rav by aggregating all receipts until they are all aggregated.

It looks like when you call stop_and_wait it's hanging trying to aggregate receipts, you need to provide a real aggregator for that test, probably.

We were previously mocking the response for the aggregator server, but since we cannot mock HTTP responses with gRPC you need to fix the test to use our new aggregator.

I found this, which seems that we can use to mock the gRPC aggregator:

https://crates.io/crates/wiremock-grpc

@pedrohba1 pedrohba1 requested a review from gusinacio January 24, 2025 05:44
@gusinacio
Copy link
Member

Closed in favor of #583

@gusinacio gusinacio closed this Jan 24, 2025
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.

2 participants