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

Create integration tests for labeler, and update toy model. #30

Merged
merged 2 commits into from
Jan 27, 2022

Conversation

tcsnfkx
Copy link
Contributor

@tcsnfkx tcsnfkx commented Jan 20, 2022

In toy model,

  • Add AttributesUpdater to set country_code.
  • Use ConditionalAssignment instead of ConditionalMerge to extract demo
    from profiles.
  • Make the profile type consistent when multiple profile types are set.
  • Format the textproto.
    Create events to test the "cookied" branch of the toy model.

This change is Reviewable

In toy model,
* Add AttributesUpdater to set country_code.
* Use ConditionalAssignment instead of ConditionalMerge to extract demo
from profiles.
* Make the profile type consistent when multiple profile types are set.
* Format the textproto.
Create events to test the "cookied" branch of the toy model.
Copy link
Contributor

@efoxepstein efoxepstein left a comment

Choose a reason for hiding this comment

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

Reviewed 29 of 29 files at r1, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EvgSkv, @kunyuchen, @lenjoy, @mpatel24, @my2457, @tcsnfkx, and @wliue)


src/test/cc/wfa/virtual_people/core/labeler/BUILD.bazel, line 23 at r1 (raw file):

    name = "labeler_integration_test",
    srcs = ["labeler_integration_test.cc"],
    data = [

Would something like this work? data = glob(["//src/test/cc/wfa/virtual_people/core/labeler/test_data/labeler_input_*.textproto", "//src/test/cc/wfa/virtual_people/core/labeler/test_data/labeler_output_*.textproto"])


src/test/cc/wfa/virtual_people/core/labeler/labeler_integration_test.cc, line 42 at r1 (raw file):

                      absl::string_view input_path,
                      absl::string_view output_path) {
  CompiledNode root;

Consider adding something like SCOPED_TRACE(absl:StrCat("ApplyAndValidate(", model_path, ", ", input_path, ", ", output_path)); to the top of this function to get better error messages.

Copy link
Collaborator

@wliue wliue left a comment

Choose a reason for hiding this comment

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

Reviewed 15 of 29 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @EvgSkv, @kunyuchen, @lenjoy, @mpatel24, @my2457, and @tcsnfkx)


src/test/cc/wfa/virtual_people/core/labeler/labeler_integration_test.cc, line 1 at r1 (raw file):

// Copyright 2021 The Cross-Media Measurement Authors

Nit: should we use 2022 now?

Code quote:

// Copyright 2021 

src/test/cc/wfa/virtual_people/core/labeler/test_data/labeler_output_01.textproto, line 2 at r1 (raw file):

people {
  virtual_person_id: 13000197489

Why there is no demo? Was the demo redistribution matrix removed from the model?

Update comments.
Add logs to the tests to get better error messages.
Apply clang-format.
Copy link
Contributor Author

@tcsnfkx tcsnfkx left a comment

Choose a reason for hiding this comment

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

Dismissed @wliue from a discussion.
Reviewable status: 28 of 29 files reviewed, all discussions resolved (waiting on @efoxepstein, @EvgSkv, @kunyuchen, @lenjoy, @mpatel24, @my2457, and @wliue)


src/test/cc/wfa/virtual_people/core/labeler/BUILD.bazel, line 23 at r1 (raw file):

Previously, efoxepstein (Eli Fox-Epstein) wrote…

Would something like this work? data = glob(["//src/test/cc/wfa/virtual_people/core/labeler/test_data/labeler_input_*.textproto", "//src/test/cc/wfa/virtual_people/core/labeler/test_data/labeler_output_*.textproto"])

It does not work.


src/test/cc/wfa/virtual_people/core/labeler/test_data/labeler_output_01.textproto, line 2 at r1 (raw file):

Previously, wliue (Wei Liu) wrote…

Why there is no demo? Was the demo redistribution matrix removed from the model?

Only label or quantum_labels of LabelerEvent are written to output (https://github.com/world-federation-of-advertisers/virtual-people-core-serving/blob/main/src/main/cc/wfa/virtual_people/core/model/population_node_impl.cc#L126). This model has demo model, but does not write the redistributed demo to label or quantum_labels. We can update the model in next PR if we want.

@tcsnfkx tcsnfkx requested a review from wliue January 25, 2022 06:31
Copy link
Collaborator

@wliue wliue 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: :shipit: complete! all files reviewed, all discussions resolved (waiting on @EvgSkv, @kunyuchen, @lenjoy, @mpatel24, and @my2457)

@tcsnfkx tcsnfkx merged commit 0e81079 into main Jan 27, 2022
@tcsnfkx tcsnfkx deleted the tcsnfkx-labeler-test branch January 27, 2022 17:42
@tcsnfkx tcsnfkx linked an issue Feb 19, 2022 that may be closed by this pull request
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.

Set up integration tests for virtual people Labeler
4 participants