Skip to content

Discarded generic method rows leave non-OnDispose event handlers on the class row's events #7373

Closed

Description

@thomhurst

Summary

A generic method row that is incompatible with a concrete instantiation can leave its builder-context handlers attached to the class row. The next compatible test then runs handlers registered for the discarded row.

Originally found while reviewing the now-closed PR #7328. Its replacement, open PR #7384 (delivering #7381), still returns the combined class/method event container from the incompatible-row branch via UnclaimedClassRow.Return. The old ReturnWithoutMethodRowAsync and SnapshotClassRowDisposeCallbacks names refer to the abandoned implementation, not current main or #7384.

Reproduction

Use a generic test method with a concrete instantiation, a class data source, and a method data source that calls RegisterForInitialization or subscribes OnTestStart. Yield an incompatible method row followed by a compatible row. The compatible test must not run the discarded row's handlers.

Acceptance criteria

  • Preserve the class row's subscriptions when returning it for reuse.
  • Drop the discarded method row's handlers from every event; raise its OnDispose callbacks exactly once without raising class-row cleanup early.
  • Cover initialization and a non-disposal lifecycle event, plus disposal, in source-generated and reflection modes.
  • Apply the fix to both eager and streaming builders while both exist; if TestBuilder.BuildTestsStreamingAsync is unused and duplicates the row-building loop #7327 has removed streaming, cover the remaining path.

Blocked by #7381, not the entire #7205 parent. No built-in data source subscribes the non-disposal handlers during a fetch, so those cases affect custom sources.

Activity

  1. added
    P2Medium: edge case, mode divergence, flaky test, or workaround exists
    on Nov 2, 2026
  2. thomhurst commented on Nov 2, 2026

    @thomhurst
    OwnerAuthor

    Triage: #7328 was closed without merging; #7384 now delivers the prerequisite row-event plumbing. Replaced the broad #7205 dependency with #7381 and updated the reproduction and acceptance criteria for #7384's UnclaimedClassRow.Return path. The discarded-row handler isolation is still needed; it is not implemented on main d8badac.

  3. thomhurst commented on Nov 5, 2026

    @thomhurst
    OwnerAuthor

    Re-check on main 17af0b5: #7381/#7384 are merged, and e30a1bb (#7476) already isolates rejected method lifecycle handlers while preserving constructor subscriptions and the rejected rows' cleanup/owning state. #7535 removed the streaming builder. The remaining gap is regression coverage for initialization and preservation of the class lifecycle handlers; the focused completion PR adds it and verifies every non-disposal event plus once-only cleanup. The expanded ClassRowCleanupTests pass all 22 cases in generated mode and all 22 in reflection mode; restoring the event leak makes both expanded cases fail on rejected initialization.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    P2Medium: edge case, mode divergence, flaky test, or workaround existsbugSomething isn't workingin-progressWork is in progress

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions