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

Update integration tests to verify direct measurement results #1212

Merged

Conversation

riemanli
Copy link
Contributor

@riemanli riemanli commented Sep 9, 2023

No description provided.

@wfa-reviewable
Copy link

This change is Reviewable

@riemanli
Copy link
Contributor Author

riemanli commented Sep 9, 2023

Current dependencies on/for this PR:

This comment was auto-generated by Graphite.

@riemanli riemanli marked this pull request as draft September 11, 2023 21:45
@riemanli riemanli requested a review from SanjayVas September 11, 2023 21:45
Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Add integration tests for direct measurements.

nit: technically this isn't adding integration tests for direct measurements, but rather updating integration tests to verify direct methodologies.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @riemanli)


src/main/kotlin/org/wfanet/measurement/loadtest/measurementconsumer/MeasurementConsumerSimulator.kt line 240 at r1 (raw file):

          reachAndFrequencyResult.reach.noiseMechanism == NoiseMechanism.CONTINUOUS_GAUSSIAN
      )
      .isTrue()

Suggestion:

    assertThat(reachAndFrequencyResult.reach.noiseMechanism)
      .isIn(listOf(NoiseMechanism.CONTINUOUS_LAPLACE, NoiseMechanism.CONTINUOUS_GAUSSIAN))

@riemanli riemanli changed the title Add integration tests for direct measurements. Update integration tests to verify direct measurement results Sep 12, 2023
@riemanli riemanli force-pushed the riemanli_kingdom_integration_test_for_direct_measurements branch from 75fb61a to dce9004 Compare September 12, 2023 22:20
@riemanli riemanli marked this pull request as ready for review September 12, 2023 22:25
@riemanli riemanli requested a review from SanjayVas September 12, 2023 22:28
Copy link
Contributor Author

@riemanli riemanli left a comment

Choose a reason for hiding this comment

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

Thanks for the suggestion. Updated the title to reflect the real change.

Reviewable status: 0 of 5 files reviewed, 1 unresolved discussion (waiting on @SanjayVas)


src/main/kotlin/org/wfanet/measurement/loadtest/measurementconsumer/MeasurementConsumerSimulator.kt line 240 at r1 (raw file):

          reachAndFrequencyResult.reach.noiseMechanism == NoiseMechanism.CONTINUOUS_GAUSSIAN
      )
      .isTrue()

Based on an offline discussion, we made the expected direct noise mechanism configurable from the constructor argument. That said, now the caller of the MC simulator needs to specified what the expected direct noise mechanism is according to their protocol config and EDP simulator setup.

Copy link
Member

@SanjayVas SanjayVas left a comment

Choose a reason for hiding this comment

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

Reviewed 5 of 5 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @riemanli)

Copy link
Member

@SanjayVas SanjayVas 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: all files reviewed, 2 unresolved discussions (waiting on @riemanli)


src/test/kotlin/org/wfanet/measurement/integration/k8s/SyntheticGeneratorCorrectnessTest.kt line 47 at r2 (raw file):

/**
 * Test for correctness of an existing CMMS on Kubernetes where the EDP simulators use
 * [SyntheticGeneratorEventQuery] with [SyntheticGenerationSpecs.POPULATION_SPEC].

I believe you need to make the same note about assuming the simulators are using ACDP composition here.

@riemanli riemanli force-pushed the riemanli_kingdom_integration_test_for_direct_measurements branch from dce9004 to 5eb5333 Compare September 12, 2023 22:49
@riemanli riemanli requested a review from SanjayVas September 12, 2023 22:49
Copy link
Contributor Author

@riemanli riemanli 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: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @SanjayVas)


src/test/kotlin/org/wfanet/measurement/integration/k8s/SyntheticGeneratorCorrectnessTest.kt line 47 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

I believe you need to make the same note about assuming the simulators are using ACDP composition here.

Thanks for catching that! Added.

Copy link
Member

@SanjayVas SanjayVas 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 r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @riemanli)

Copy link
Contributor

@Marco-Premier Marco-Premier left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @stevenwarejones)

Copy link
Collaborator

@stevenwarejones stevenwarejones left a comment

Choose a reason for hiding this comment

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

Reviewed 4 of 5 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @riemanli)

@riemanli riemanli force-pushed the riemanli_kingdom_integration_test_for_direct_measurements branch from 5eb5333 to b9e7911 Compare September 14, 2023 16:22
@riemanli riemanli enabled auto-merge (squash) September 14, 2023 16:22
@riemanli riemanli merged commit 247d18b into main Sep 14, 2023
@riemanli riemanli deleted the riemanli_kingdom_integration_test_for_direct_measurements branch September 14, 2023 17:03
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.

5 participants