Check Measurement ConsumerId in EdpSimulator

[uakyol]

No description provided.

[wfa-reviewable]

This change is 

[SanjayVas]

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

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 388 at r2 (raw file):

    val requisitions =
      getRequisitions().filter {
        MeasurementKey.fromName(it.measurement)!!.measurementConsumerId == measurementConsumerName

This is comparing and ID to a name. Just directly compare it.measurement to measurementConsumerName

[uakyol]

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

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 388 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

This is comparing and ID to a name. Just directly compare it.measurement to measurementConsumerName

ah sorry, didn't see your comment and updated based on lifeofameasurement test failing. How about this?

[SanjayVas]

Reviewed all commit messages.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @uakyol)

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 388 at r2 (raw file):

Previously, uakyol wrote…

ah sorry, didn't see your comment and updated based on lifeofameasurement test failing. How about this?

Ah, I also made a mistake in my comment. Parse both names to their respective keys, then compare the MC ID fields.

[uakyol]

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

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 388 at r2 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Ah, I also made a mistake in my comment. Parse both names to their respective keys, then compare the MC ID fields.

Done.

[SanjayVas]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @uakyol)

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 389 at r4 (raw file):

    val requisitions =
      getRequisitions().filter {
        MeasurementKey.fromName(it.measurement)!!.measurementConsumerId ==

nit: Use the appropriate precondition check rather than !!. That operator is for cases where we can guarantee it won't be null, but we just can't prove it to the compiler. There is no code prior to this point that has proven that this value won't be null.

Code quote:

!!

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 399 at r4 (raw file):

    for (requisition in requisitions) {
      println("requisitionrequisitionrequisitionrequisitionrequisition ${requisition.measurement}")

Use a logger if you need to log something.

Code quote:

println

---

src/test/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulatorTest.kt line 300 at r4 (raw file):

  fun setup() {
    sketchStore = SketchStore(FileSystemStorageClient(temporaryFolder.root))
  }

Why was this changed? It used to be a @BeforeClass for a JVM static member (and had a more descriptive function name).

Code quote:

  @Before
  fun setup() {
    sketchStore = SketchStore(FileSystemStorageClient(temporaryFolder.root))
  }

[uakyol]

Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @SanjayVas)

---

src/main/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulator.kt line 399 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Use a logger if you need to log something.

sorry, meant to remove

---

src/test/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulatorTest.kt line 300 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Why was this changed? It used to be a @BeforeClass for a JVM static member (and had a more descriptive function name).

If I do it that way, the test fails with null check failing. Correct me if I am wrong but I think this is because it is initialized once before the class and not before each test case. This causes other test cases to write to that temp folder and those writes being visible to this new test

[SanjayVas]

Reviewed 1 of 1 files at r5.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @Marco-Premier and @uakyol)

---

src/test/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulatorTest.kt line 300 at r4 (raw file):

Previously, uakyol wrote…

If I do it that way, the test fails with null check failing. Correct me if I am wrong but I think this is because it is initialized once before the class and not before each test case. This causes other test cases to write to that temp folder and those writes being visible to this new test

Whether that's right depends on the test design. e.g. if each test method inherently deals with different blob keys, then having the temp directory initialized once is fine.

If you want to switch from @BeforeClass to @Before, just make sure to keep the same descriptive names. The whole reason JUnit4 switched to annotated methods is to avoid having the non-descript "setup" and "teardown" methods of JUnit3.

[uakyol]

src/test/kotlin/org/wfanet/measurement/loadtest/dataprovider/EdpSimulatorTest.kt line 300 at r4 (raw file):

Previously, SanjayVas (Sanjay Vasandani) wrote…

Whether that's right depends on the test design. e.g. if each test method inherently deals with different blob keys, then having the temp directory initialized once is fine.

If you want to switch from @BeforeClass to @Before, just make sure to keep the same descriptive names. The whole reason JUnit4 switched to annotated methods is to avoid having the non-descript "setup" and "teardown" methods of JUnit3.

thanks, I kept it as @Before and updated the "setup" method name to "initSketchStore" thats what you were referring to right?

[SanjayVas]

Reviewed 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Marco-Premier)

[stevenwarejones]

Reviewed 1 of 1 files at r5, 1 of 1 files at r6, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @Marco-Premier)