Follow-up from "test(managementapi): cover the container upstream associate refusals and update part 13/15 (S17 Phase 6 plan: 42/42)"

The following discussions from !2858 (merged) should be addressed:

  • @vespian_gl started a discussion:

    non-blocking: This test does not build a foreign upstream. Both iterations set the same associateErr (datastore.ErrVirtualUpstreamNotFound) under the default evaluator of the rig, which reads every upstream as readable. For an upstream in another namespace, the one input that differs at the handler is the upstreamReadable verdict, which runs before arm.store.Associate. Thus, if the handler starts to answer an unreadable upstream differently, this test still passes. TestContainerUpstreams_Associate_OmitsAnUnreadableUpstreamsFields covers only part of that. Also, the doc comment says that the responses are "compared byte for byte past the per-request id", but the test compares only code|message.

    TestContainerUpstreams_Associate_RefusesSelfReferenceAndTombstonedUpstream at line 788 has the same pattern. The fake returns associateErr before its splice, whatever id arrives, and the rig seeds no rows. Thus assert.Empty(tt, rig.store.Rows()) at line 829 is true by construction.

    For the foreign iteration, you should install readableUpstreamsEvaluator() with no readable ids. Then compare rec.Code and the whole rec.Body, or change the comment to say that it compares the code and the message only. For the self-reference table, you can remove the Rows() assertion, or say in the comment that the store owns the refusal and this table pins only the error mapping. The npm twins have the same shape, so the same edits apply there if you want the pair to stay the same.

    This is a bot message 🤖 — /mergeable

  • @vespian_gl started a discussion:

    non-blocking: This assertion cannot fail on any row of the table. The rig seeds one row, and a list of one row cannot show a move. On the rows that set repositionErr, the fake returns before it changes the list. On the two 400 rows, a handler that skipped the 400 calls Reposition with position 1. The fake computes min(1, 1) - 1 = 0 and leaves the row at position 1. The status assertion catches that regression, so this line adds no coverage of its own.

    On the rows that set no err, you should assert assert.Empty(tt, rig.store.Repositions()), because the fake records each call. Another option is to seed two rows and send a position that moves row 1. TestNpmUpstreams_Update_RejectsBadRequests has the same assertion on the same fixture, so the same change applies there if you want the dupl pair to stay the same.

    This is a bot message 🤖 — /mergeable

  • @vespian_gl started a discussion:

    non-blocking: This test seeds the same two rows as TestContainerUpstreams_List_ReturnsThePositionOrderedSet at line 135, and it sends the same GET. The older test calls assertUpstreamResource on both rows. That helper already compares summary["format"] with wantSummaryFormats[want.UpstreamFormat]. Thus no handler change fails this test and passes the older one.

    You should remove this test, or give it a state that the older test does not build.

    This is a bot message 🤖 — /mergeable