test: make the courseware redirect-rule tests actually assert

[brian-smith-tcril]

Summary

Make the 13 async redirect-rule tests in CoursewareContainer.test.jsx actually assert. Their waitFor calls were never awaited, so every assertion inside them was vacuous, and most asserted on local jest.fn() mocks that nothing ever called. Test-only; CoursewareContainer.tsx is untouched. Stacked below #2074, which moves these tests into redirects.test.ts — with this layer underneath, the moved tests arrive already correct. Part of #1946 as test hygiene on code the migration touches. Closes #2078.

What changed

• Awaited the 13 waitFor calls and made their tests async: checkUnitToSequenceUnitRedirect (4 + 3 tests across the isPreview true/false blocks) and checkResumeRedirect (4 + 2).
• Replaced the unwired mocks. Six const getSequenceForUnitDeprecated = jest.fn() and two const getResumeBlock = jest.fn() declarations were never passed to the code under test, so expect(mock).toHaveBeenCalled() could never pass. They now assert on the request the rule actually makes (axiosMock.history.get contains the endpoint) or, for the not-a-unit case, that no request was made. The two resume "returns" cases wait for the request, flush a tick, then assert navigate was not called.
• Corrected four expectations that never matched the rule, verified against the rule at feat: add functionality to see unit draft preview #1501 (when the tests were written) and today — the rule has not changed in between:
  • isPreview true › unit-to-sequence: expected /course/courseId/sequence_1; the rule navigates to the parent sequence and the unit, with the /preview prefix the same PR introduced → /preview/course/courseId/sequence_1/unit_1.
  • isPreview true › resume "returns after calling getResumeBlock": passed firstSequenceId: 'sequence_1', so the rule's fallback navigated there; now passes null so the case tests the no-navigation path it names.
  • isPreview false › resume "calls navigate with unitId": expected a /preview URL with isPreview false → /course/courseId/section_1/unit_1.
  • isPreview false › unit-to-sequence: mocked a parent: {...} response shape the api has not read since feat: stop calling course blocks rest API and assume LS exists #803 (2022), so the rule hit its error branch; now mocks the blocks shape, passes isPreview: false as the block name says, and expects /course/courseId/sequence_1/unit_1. Its apiUrl is now the href string like the sibling block, so the request assertion can match.

Testing

npm run types (0 errors), npm run lint (clean); CoursewareContainer.test.jsx 70/70 on this layer. With #2074 rebased on top, redirects.test.ts carries the same fixes and passes 51/51 with every waitFor awaited.

Provenance

Introduced with the tests themselves in #1501 (2024-10-28); untouched since, so the redirect rules the migration extracted in #2074 had never actually been exercised by these tests. Flagged by @arbrandes in review of #2074 (comment); split out as its own layer below #2074 so that PR stays a faithful move and this change is reviewable on its own.

Decisions

Full decision log

Decisions — make the courseware redirect-rule tests actually assert (#2078)

1. A layer below refactor: tear down the courseware Redux slice #2074, not inside it and not a PR against master. The
   13 un-awaited waitFor calls predate the migration (feat: add functionality to see unit draft preview #1501, 2024-10-28) and
   refactor: tear down the courseware Redux slice #2074 only moves them, so fixing them inside refactor: tear down the courseware Redux slice #2074 would blur a faithful
   move with a test rewrite. A plain PR against master would keep refactor: tear down the courseware Redux slice #2074 clean
   too, but refactor: tear down the courseware Redux slice #2074 would only pick the fix up after that PR landed; as a stack
   layer underneath, refactor: tear down the courseware Redux slice #2074 rebases onto it now and lands with the moved tests
   already correct. So: gh stack unstack, gh stack init --base master bsmith/redirect-tests-assert bsmith/courseware-slice-teardown progress-exam-attempts-query, fix on the new bottom, gh stack rebase --no-trunk. The fix is therefore written against master's file
   (CoursewareContainer.test.jsx, positional check* signatures); refactor: tear down the courseware Redux slice #2074's
   move carries it into redirects.test.ts (options-object signatures) via
   the conflict resolution: take refactor: tear down the courseware Redux slice #2074's side of the container file (the
   tests leave it) and re-apply the same corrections to redirects.test.ts
   from a patch prepared earlier in refactor: tear down the courseware Redux slice #2074's shape.

2. await alone would have turned vacuous tests into failing ones: the
   unwired mocks become request assertions on axiosMock.history. Six
   tests declared const getSequenceForUnitDeprecated = jest.fn(); and two
   const getResumeBlock = jest.fn(); — fresh local mock functions that
   merely share a name with the api functions. Nothing connected them:
   not passed to the rule, no jest.mock of the module, no jest.spyOn on
   the export. The rules import the real functions from ./data/api at
   module load and call those regardless of what a local variable in the
   test is named, so expect(mock).toHaveBeenCalled() asked whether an
   unused local was called — always no, hidden only because the un-awaited
   waitFor dropped the failure.

   What the author wanted to check — "the rule looked up the parent sequence
   / fetched the resume block" — is observable one layer down without any
   mock on the function: the real api function's only side effect before
   parsing is getAuthenticatedHttpClient().get(url), and the tests already
   intercept that transport with axiosMock.onGet(...). axios-mock-adapter
   records each handled request in axiosMock.history.get (a fresh adapter
   per test in beforeEach, so the history is per test), so the assertion
   becomes "a GET to the endpoint is in the history":
   expect(axiosMock.history.get.map((req) => req.url)).toContain(apiUrl).
   The not-a-unit branch must short-circuit without touching the network, so
   its not.toHaveBeenCalled() became expect(axiosMock.history.get).toHaveLength(0)
   — stronger than the original would have been even if wired, since it rules
   out any request.

   Why not wire the mock properly instead: jest.mock('./data/api') would
   replace the module for the whole file, and the container-rendering tests in
   the same file drive the real api functions against the mocked network;
   jest.spyOn on the namespace import works under Babel's CJS interop but is
   fragile and would still need a mockImplementation to keep the real lookup
   (or no parent is ever found). The history check needs no module surgery and
   exercises the real function end to end — including its
   Object.values(data.blocks) parsing, which is what exposed the
   parent: {...} response mocks in the other block as never having matched
   the api (entry 3). It also required the second block's apiUrl to be the
   href string like the first block's, not a URL object, since history
   entries store the URL as a string.

   The two checkResumeRedirect "returns" cases wait for the request, flush
   one macrotask (setTimeout(resolve, 0)) so the .then after the mocked
   response has run, then assert navigate was not called — asserting "not
   called" inside waitFor would pass trivially on the first poll.

3. The rule is the source of truth for the four expectations that had never
   matched it. git log -S shows checkUnitToSequenceUnitRedirect's and
   checkResumeRedirect's navigate targets unchanged since feat: add functionality to see unit draft preview #1501 (only
   refactor: tear down the courseware Redux slice #2074's move touches those lines), and the pre-feat: add functionality to see unit draft preview #1501 version already
   navigated unit-to-sequence to /course/:courseId/:parentId/:unitId. So
   every correction is test-side. Direction was decided by reachability and by
   the describe block each test sits in:

   • Unit id (isPreview true › unit-to-sequence). Expected
     /course/courseId/sequence_1. The rule has exactly three navigate
     targets — ${sequenceUrl}/${unitId}, or /course/:courseId in the
     no-parent and not-a-unit branches — and none of them is a parent-sequence
     URL without the unit, so no argument values could reach the original
     expectation; the URL side had to be wrong. The test's own name, "parentId
     and sequenceId", is the pair the rule joins (sequenceId here is
     unit_1). Corrected to include /unit_1.
   • /preview prefix (same test). The prefix depends only on the ninth
     argument, isPreview, which the test passes as true, and the test sits
     in describe('isPreview equals true') — the block feat: add functionality to see unit draft preview #1501 created for the
     prefixed branch it introduced. Params and block agree the scenario is
     preview-on, so the URL must carry /preview. Flipping the argument
     instead would make a test in the "true" block exercise the "false"
     branch, which the sibling block covers. Corrected to
     /preview/course/courseId/sequence_1/unit_1.
   • Where params and block disagree, the block wins (isPreview false ›
     unit-to-sequence). Its three tests passed true for isPreview inside
     describe('isPreview equals false'). The block is the author's stated
     intent; the argument is a copy-paste artifact. Flipped to false; the
     first test now expects /course/courseId/sequence_1/unit_1. Between the
     two blocks both prefix branches are exercised, which the original never
     managed.
   • Response shape (isPreview false › unit-to-sequence). Those mocks
     answered parent: { id } / parent: { children }. getSequenceForUnitDeprecated
     has read Object.values(data.blocks) since feat: stop calling course blocks rest API and assume LS exists #803 (2022-02-17), two years
     before the tests were written, so the "found parent" case threw inside
     the api and hit the error branch — the only reason its sibling "no parent
     id" case coincidentally passed. Both now mock the blocks shape the api
     reads; the "no parent" case exercises the real no-parent branch (the
     apiUrl type fix is in entry 2).
   • Resume "returns after calling getResumeBlock" (isPreview true).
     Passed firstSequenceId: 'sequence_1' with a response carrying neither
     sectionId nor unitId, so the rule's else if (firstSequenceId)
     fallback navigated to /course/courseId/sequence_1 — the test's premise
     ("returns") contradicted its inputs. Now passes null, the one input
     under which the rule does nothing, which is what the sibling "calls
     navigate with firstSequenceId" case already covers from the other side.
   • Resume "calls navigate with unitId" (isPreview false). Expected
     /preview/course/courseId/section_1/unit_1 with isPreview: false;
     the rule prefixes only when true. Corrected to
     /course/courseId/section_1/unit_1.

4. Test-only. CoursewareContainer.tsx and redirects.ts are untouched;
   the diff is one file (56 insertions, 55 deletions) on this layer, and
   redirects.test.ts differs from its upstream version by the equivalent 55/54
   once refactor: tear down the courseware Redux slice #2074 is rebased on top. Lint and types clean; CoursewareContainer.test.jsx
   70/70 here, redirects.test.ts 51/51 at the top with all 13 waitFor
   calls awaited and no jest.fn() in the file other than navigate.

5. Branch name carries the bsmith/ namespace. Stack layers push to the
   openedx upstream remote, so the branch is bsmith/redirect-tests-assert
   (first init used the bare name; re-done). progress-exam-attempts-query
   and the parked retire-course-home-slice predate that rule.

6. gh stack unstack left the merged history grouped. It reported that
   some PRs were "queued for merge or have auto-merge enabled" and left local
   tracking alone, but on GitHub the two open PRs did come out of the stack
   while the 13 merged ones stayed grouped — which is what we wanted. Neither
   open PR was queued or auto-merging; the message appears to be the tool
   misreading the merged PRs. Local tracking was then dropped with
   --local and re-created with init.

🤖 Generated with Claude Code

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.78%. Comparing base (548688d) to head (90a5637).

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2079   +/-   ##
=======================================
  Coverage   93.78%   93.78%           
=======================================
  Files         367      367           
  Lines        6048     6048           
  Branches     1433     1433           
=======================================
  Hits         5672     5672           
  Misses        359      359           
  Partials       17       17           

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[arbrandes]

👍🏼