Marcopremier fix confirmcomputationparticipant

[Marco-Premier]

PR to solve issue #1269

[wfa-reviewable]

This change is 

[uakyol]

The code looks good to me thanks for the quick fix Marco.
We also need to update the unit tests for this change. Can you add it to this PR please?

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @SanjayVas and @stevenwarejones)

[Marco-Premier]

Hey @uakyol just added the test :)

Reviewable status: 1 of 3 files reviewed, all discussions resolved (waiting on @SanjayVas and @stevenwarejones)

[uakyol]

thank you :), added some minor comments.

Reviewed 2 of 2 files at r2, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Marco-Premier, @SanjayVas, and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/ComputationParticipantsServiceTest.kt line 704 at r2 (raw file):

  @Test
  fun `confirmComputationParticipant succeeds with two duchies`() = runBlocking {

What do you think about changing the name to confirmComputationParticipant succeeds with a subset of registered duchies since 2 is arbitrary, we are trying to test if this would work if not all duchies are included.

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/ComputationParticipantsServiceTest.kt line 705 at r2 (raw file):

  @Test
  fun `confirmComputationParticipant succeeds with two duchies`() = runBlocking {
    val duchies = DUCHIES.take(2)

consider using droplast() (https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/drop-last.html).
This way if someone changes the DUCHIES in the Population class to 2 duchies this test will still be correct.

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/Population.kt line 90 at r2 (raw file):

    val WORKER1_DUCHY = DuchyIds.Entry(2, "worker1", VALID_ACTIVE_START_TIME..VALID_ACTIVE_END_TIME)
    val WORKER2_DUCHY = DuchyIds.Entry(3, "worker2", VALID_ACTIVE_START_TIME..VALID_ACTIVE_END_TIME)
    val DUCHIES = listOf(AGGREGATOR_DUCHY, WORKER1_DUCHY, WORKER2_DUCHY)

Consider adding an inactive duchy?
There are lots of other tests using this object and after those tests are refactored to account for this change. The functionality of removing a duchy should work. As things stand, we caught this bug but there might be others. This change would uncover more if any.

For example if we had an inactive duchy here, the above tests would have failed.

wdyt? If you think this is a good idea too, feel free to tackle this in another PR.

[Marco-Premier]

Reviewable status: 2 of 3 files reviewed, 3 unresolved discussions (waiting on @SanjayVas, @stevenwarejones, and @uakyol)

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/ComputationParticipantsServiceTest.kt line 704 at r2 (raw file):

Previously, uakyol wrote…

What do you think about changing the name to confirmComputationParticipant succeeds with a subset of registered duchies since 2 is arbitrary, we are trying to test if this would work if not all duchies are included.

Done.

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/ComputationParticipantsServiceTest.kt line 705 at r2 (raw file):

Previously, uakyol wrote…

consider using droplast() (https://kotlinlang.org/api/latest/jvm/stdlib/kotlin.collections/drop-last.html).
This way if someone changes the DUCHIES in the Population class to 2 duchies this test will still be correct.

yeah I agree even though some constraint will remain. e.g. if someone change the order of DUCHIES ad put the aggregator as last element in the list, test would fail.

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/Population.kt line 90 at r2 (raw file):

Previously, uakyol wrote…

Consider adding an inactive duchy?
There are lots of other tests using this object and after those tests are refactored to account for this change. The functionality of removing a duchy should work. As things stand, we caught this bug but there might be others. This change would uncover more if any.

For example if we had an inactive duchy here, the above tests would have failed.

wdyt? If you think this is a good idea too, feel free to tackle this in another PR.

I see your point. I think it would be even more helpful to add a duchy which is active but is not a "required duchy" in any data provider.
Inactive duchy are automatically "dropped" when a measurement is created instead active ones my be added to the list of participant if the minimum number of required duchies is not met.

I'll follow up in a different PR :)

[SanjayVas]

Reviewed 1 of 1 files at r1, 1 of 2 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @Marco-Premier, @stevenwarejones, and @uakyol)

---

src/main/kotlin/org/wfanet/measurement/kingdom/deploy/gcloud/spanner/writers/ConfirmComputationParticipant.kt line 145 at r3 (raw file):

  }

  private suspend fun TransactionScope.getComputationParticipants(

nit

Suggestion:

getComputationParticipantDuchyIds

[uakyol]

Reviewed 1 of 1 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Marco-Premier and @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/Population.kt line 90 at r2 (raw file):

Previously, Marco-Premier (marcopremier) wrote…

I see your point. I think it would be even more helpful to add a duchy which is active but is not a "required duchy" in any data provider.
Inactive duchy are automatically "dropped" when a measurement is created instead active ones my be added to the list of participant if the minimum number of required duchies is not met.

I'll follow up in a different PR :)

awesome thanks :)

[SanjayVas]

Waiting on this for the 0.4.1 patch release.

[Marco-Premier]

Reviewed 1 of 2 files at r2, 1 of 1 files at r3, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @stevenwarejones)

[Marco-Premier]

Dismissed @uakyol from a discussion.
Reviewable status: complete! all files reviewed, all discussions resolved (waiting on @stevenwarejones)

---

src/main/kotlin/org/wfanet/measurement/kingdom/service/internal/testing/Population.kt line 90 at r2 (raw file):

Previously, uakyol wrote…

awesome thanks :)

Done.

[stevenwarejones]

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