Holdings test with 3 suppliers as result

[adamdickmeiss]

No description provided.

[Copilot]

Pull request overview

Adds a new end-to-end holdings test that exercises the path where an SRU consortium lookup returns a single bibliographic record with three holdings (DE-89, DE-468, DE-Kob7), so the broker iterates through three suppliers and eventually completes the loan.

Changes:

• New TestRequestRequestSruServerLoanedMultiple test plus a loadResponse3 atomic toggle in the SRU mock handler to serve a different canned response; existing tests are updated to explicitly reset the new flag.
• Adds the three corresponding library entries (DE-89, DE-468, DE-Kob7) to the GVI test directory and a new ILL request fixture (request-5.xml).
• Adds the new SRU mock response file (gvi_sru_response_3.xml) containing the multi-holdings MARC record.
• Tightens assertions in TestRequestRequestSruServerLoaned to expect the final LoanCompleted / ShippedReturn states rather than the initial Request state.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
broker/test/holdings/holdings_test.go	Adds toggle for alternate SRU response, new multi-supplier test, and stricter assertions on the loaned test.
broker/test/holdings/request-5.xml	New ISO18626 patron request fixture used by the new test.
broker/test/holdings/gvi_sru_response_3.xml	New SRU mock response with three 924 holdings entries.
broker/test/holdings/gvi_directory.json	Adds three institution entries (DE-89, DE-468, DE-Kob7) needed by the new test scenario.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (2)

broker/test/holdings/holdings_test.go:326

• TestRequestRequestSruServerLoanedMultiple is almost a verbatim copy of TestRequestRequestSruServerLoaned (request setup, polling loop, assertion of LoanCompleted/ShippedReturn, and the prefix of the event log). Consider extracting the common boilerplate (HTTP request execution, predicate wait, status assertions) into a helper and only parameterize the request file, reqId, and expected event log. This will keep future protocol/event-flow tweaks from requiring parallel edits in multiple places.

func TestRequestRequestSruServerLoanedMultiple(t *testing.T) {
	shouldFailSruRequest.Store(false)
	loadResponse3.Store(true)
	t.Cleanup(func() {
		loadResponse3.Store(false)
	})
	appCtx := common.CreateExtCtxWithArgs(context.Background(), nil)
	reqId := "11deaad0-e492-4cc7-9527-6713466cc434"
	data, err := os.ReadFile("request-5.xml")
	assert.NoError(t, err, "failed to read request file")
	req, err := http.NewRequest("POST", mockPeerUrl, bytes.NewReader(data))
	assert.NoError(t, err, "failed to create request")
	req.Header.Add("Content-Type", "application/xml")
	client := &http.Client{}
	res, err := client.Do(req)
	assert.NoError(t, err, "failed to send request to mock")
	assert.Equal(t, http.StatusOK, res.StatusCode, "handler returned wrong status code")
	var illTrans ill_db.IllTransaction
	test.WaitForPredicateToBeTrue(func() bool {
		illTrans, err = illRepo.GetIllTransactionByRequesterRequestId(appCtx, getPgText(reqId))
		if err != nil {
			t.Errorf("failed to find ill transaction by requester request id %v", reqId)
		}
		return illTrans.LastSupplierStatus.String == "LoanCompleted" &&
			illTrans.LastRequesterAction.String == "ShippedReturn"
	})
	assert.Equal(t, "LoanCompleted", illTrans.LastSupplierStatus.String)
	assert.Equal(t, "ShippedReturn", illTrans.LastRequesterAction.String)
	exp := "NOTICE, request-received = SUCCESS\n" +
		"TASK, locate-suppliers = SUCCESS\n" +
		"TASK, select-supplier = SUCCESS\n" +
		"TASK, check-availability = SUCCESS\n" +
		"TASK, message-requester = SUCCESS\n" +
		"TASK, message-supplier = SUCCESS\n" +
		"NOTICE, supplier-msg-received = SUCCESS\n" +
		"TASK, message-requester = SUCCESS\n" +
		"TASK, confirm-supplier-msg = SUCCESS\n" +
		"TASK, select-supplier = SUCCESS\n" +
		"TASK, check-availability = SUCCESS\n" +
		"TASK, message-requester = SUCCESS\n" +
		"TASK, message-supplier = SUCCESS\n" +
		"NOTICE, supplier-msg-received = SUCCESS\n" +
		"TASK, message-requester = SUCCESS\n" +
		"TASK, confirm-supplier-msg = SUCCESS\n" +
		"NOTICE, requester-msg-received = SUCCESS\n" +
		"TASK, message-supplier = SUCCESS\n" +
		"TASK, confirm-requester-msg = SUCCESS\n" +
		"NOTICE, requester-msg-received = SUCCESS\n" +
		"TASK, message-supplier = SUCCESS\n" +
		"TASK, confirm-requester-msg = SUCCESS\n" +
		"NOTICE, supplier-msg-received = SUCCESS\n" +
		"TASK, message-requester = SUCCESS\n" +
		"TASK, confirm-supplier-msg = SUCCESS\n"
	apptest.EventsCompareString(appCtx, eventRepo, t, illTrans.ID, exp)

broker/test/holdings/holdings_test.go:278

• shouldFailSruRequest is only reset at the start of each test, but loadResponse3 here is wrapped with t.Cleanup. If a future test is added that does not explicitly call loadResponse3.Store(false) (mirroring how the other tests rely on the implicit false default for shouldFailSruRequest), the cleanup here helps — but the asymmetric handling between the two atomic flags is easy to get wrong. Consider applying the same t.Cleanup pattern (or a shared reset helper) for shouldFailSruRequest so the test-isolation strategy is consistent.

	shouldFailSruRequest.Store(false)
	loadResponse3.Store(true)
	t.Cleanup(func() {
		loadResponse3.Store(false)
	})

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

Comments suppressed due to low confidence (1)

broker/test/holdings/holdings_test.go:278

• This test mutates the package-global loadResponse3 and relies on t.Cleanup to reset it. If any other test in this package is later marked with t.Parallel() (or this one is), the shared flag — together with the equally shared shouldFailSruRequest — will cause cross-test interference and flakiness, because the SRU handler is a single goroutine-shared dispatcher keyed off these globals. Consider routing the desired response through the request itself (e.g., a header or a per-test path) or guarding the toggle with a mutex/sequential gate, so future parallelization doesn't silently corrupt results.

func TestRequestRequestSruServerLoanedMultiple(t *testing.T) {
	shouldFailSruRequest.Store(false)
	loadResponse3.Store(true)
	t.Cleanup(func() {
		loadResponse3.Store(false)
	})