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

Hybrid integration test follow up #1479

Conversation

eriktaubeneck
Copy link
Member

@eriktaubeneck eriktaubeneck commented Dec 5, 2024

adds an integration test for the hybrid protocol, plus a bunch of bug fixes to get it to work

@tyurek
Copy link
Collaborator

tyurek commented Dec 5, 2024

quick question, which encrypt command are you calling in test_hybrid()?

@eriktaubeneck eriktaubeneck force-pushed the hybrid-integration-test-follow-up branch from 7925b17 to 2b06be6 Compare December 5, 2024 16:03
@eriktaubeneck
Copy link
Member Author

quick question, which encrypt command are you calling in test_hybrid()?

@tyurek I was calling encrypt (knowing it would be incorrect), but I just updated it to call hybrid-encrypt.

failure is the same, and has to do with how the network.toml is loaded in. . I think we just need to use the sharded version, looking into it.

@eriktaubeneck
Copy link
Member Author

ok, just fixed the issue with the network.toml file in the encrypt-hybrid cli command. still getting it with the helper binary. investigating.

@eriktaubeneck
Copy link
Member Author

got the network.toml parsing working! (needed to pass in a shard count to the report_collector).

the test is still failing, though:

IPA results file is valid JSON: Error("invalid type: integer `5`, expected struct HybridQueryParams", line: 3, column: 3)

@eriktaubeneck eriktaubeneck mentioned this pull request Dec 6, 2024
@eriktaubeneck eriktaubeneck marked this pull request as ready for review December 6, 2024 18:08
Copy link

codecov bot commented Dec 6, 2024

Codecov Report

Attention: Patch coverage is 7.69231% with 96 lines in your changes missing coverage. Please review.

Project coverage is 93.20%. Comparing base (e667e78) to head (700646c).
Report is 6 commits behind head on main.

Files with missing lines Patch % Lines
ipa-core/src/bin/report_collector.rs 2.63% 37 Missing ⚠️
ipa-core/src/query/runner/hybrid.rs 0.00% 24 Missing ⚠️
ipa-core/src/cli/playbook/hybrid.rs 0.00% 17 Missing ⚠️
ipa-core/src/query/executor.rs 0.00% 16 Missing ⚠️
ipa-core/src/net/http_serde.rs 33.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1479      +/-   ##
==========================================
- Coverage   93.37%   93.20%   -0.17%     
==========================================
  Files         239      239              
  Lines       43476    43547      +71     
==========================================
- Hits        40594    40590       -4     
- Misses       2882     2957      +75     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ipa-core/src/bin/report_collector.rs Outdated Show resolved Hide resolved
ipa-core/src/protocol/basics/shard_fin.rs Show resolved Hide resolved
@eriktaubeneck eriktaubeneck merged commit fcfdfa6 into private-attribution:main Dec 6, 2024
12 of 13 checks passed
@eriktaubeneck eriktaubeneck deleted the hybrid-integration-test-follow-up branch December 6, 2024 22:27
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.

3 participants