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

Replaced internal encoders with message encoder calling public API #767

Merged
merged 27 commits into from
Jul 24, 2024

Conversation

LazeringDeath
Copy link
Contributor

@LazeringDeath LazeringDeath commented Jun 21, 2024

What does this Pull Request accomplish?

Replaced the Google's internal encoders with message encoders without affecting performance or results.

Why should this Pull Request be merged?

Resolve issue#35 and make the serializer more robust/less complicated.

What testing has been done?

Passed all measurementlink tests and PR checks.

Copy link

github-actions bot commented Jun 21, 2024

Test Results

    30 files  ±  0      30 suites  ±0   43m 7s ⏱️ - 2m 55s
   632 tests  -  21     632 ✅  -  21      0 💤 ±0  0 ❌ ±0 
15 580 runs   - 630  14 520 ✅  - 630  1 060 💤 ±0  0 ❌ ±0 

Results for commit e76e00a. ± Comparison against base commit ac37644.

This pull request removes 41 and adds 20 tests. Note that renamed tests count towards both.
tests.unit.test_serialization_strategy ‑ test___serialization_strategy___get_decoder___returns_expected_decoder[1-False--scalar_decoder]
tests.unit.test_serialization_strategy ‑ test___serialization_strategy___get_decoder___returns_expected_decoder[11-False-ni.protobuf.types.DoubleXYData-message_decoder]
tests.unit.test_serialization_strategy ‑ test___serialization_strategy___get_decoder___returns_expected_decoder[11-True-ni.protobuf.types.DoubleXYData-message_decoder]
tests.unit.test_serialization_strategy ‑ test___serialization_strategy___get_decoder___returns_expected_decoder[13-False--scalar_decoder]
tests.unit.test_serialization_strategy ‑ test___serialization_strategy___get_decoder___returns_expected_decoder[14-False--scalar_decoder]
tests.unit.test_serialization_strategy ‑ test___serialization_strategy___get_decoder___returns_expected_decoder[2-False--scalar_decoder]
tests.unit.test_serialization_strategy ‑ test___serialization_strategy___get_decoder___returns_expected_decoder[3-False--scalar_decoder]
tests.unit.test_serialization_strategy ‑ test___serialization_strategy___get_decoder___returns_expected_decoder[4-False--scalar_decoder]
tests.unit.test_serialization_strategy ‑ test___serialization_strategy___get_decoder___returns_expected_decoder[5-False--scalar_decoder]
tests.unit.test_serialization_strategy ‑ test___serialization_strategy___get_decoder___returns_expected_decoder[8-False--scalar_decoder]
…
tests.unit.test_decoder ‑ test___big_message___deserialize_parameters___returns_parameter_value_by_id
tests.unit.test_decoder ‑ test___empty_buffer___deserialize_parameters___returns_zero_or_empty
tests.unit.test_decoder ‑ test___serializer___deserialize_parameter___successful_deserialization[values0]
tests.unit.test_default_value ‑ test___get_default_value___returns_type_defaults[1-False-0.0]
tests.unit.test_default_value ‑ test___get_default_value___returns_type_defaults[11-False-None]
tests.unit.test_default_value ‑ test___get_default_value___returns_type_defaults[11-True-expected_default_value10]
tests.unit.test_default_value ‑ test___get_default_value___returns_type_defaults[13-False-0]
tests.unit.test_default_value ‑ test___get_default_value___returns_type_defaults[14-False-0]
tests.unit.test_default_value ‑ test___get_default_value___returns_type_defaults[2-False-0.0]
tests.unit.test_default_value ‑ test___get_default_value___returns_type_defaults[3-False-0]
…

♻️ This comment has been updated with latest results.

@LazeringDeath LazeringDeath marked this pull request as ready for review June 25, 2024 13:19
@LazeringDeath LazeringDeath changed the title [DRAFT] Replaced internal encoders with message encoder calling public API Replaced internal encoders with message encoder calling public API Jun 25, 2024
@bkeryan
Copy link
Collaborator

bkeryan commented Jun 25, 2024

Please update the PR description.

Edit: oh, I see, you did, but you kept the TODOs. Delete the TODOs.

Copy link
Collaborator

@bkeryan bkeryan left a comment

Choose a reason for hiding this comment

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

I'm not done reviewing this. FYI, my team is doing release finalization now, so I might not get back to this for a few days.

@dixonjoel dixonjoel requested a review from bkeryan July 1, 2024 20:37
@dixonjoel dixonjoel requested a review from bkeryan July 15, 2024 15:59
@dixonjoel dixonjoel self-requested a review July 15, 2024 19:17
@dixonjoel
Copy link
Collaborator

@LazeringDeath generally looks good. I'll take a more thorough look when all the checks are passing.

Copy link
Collaborator

@bkeryan bkeryan left a comment

Choose a reason for hiding this comment

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

I think you're on the home stretch.

Copy link
Collaborator

@bkeryan bkeryan left a comment

Choose a reason for hiding this comment

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

Hopefully this is the last round.

@LazeringDeath LazeringDeath merged commit aa99a84 into main Jul 24, 2024
17 checks passed
@LazeringDeath LazeringDeath deleted the users/tynguyen/encoder-messages branch July 24, 2024 19:59
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.

Research alternatives to google.protobuf.internal encoder and decoder
4 participants