Skip to content

junit-core: add "Asserting Optional values" guidance (avoid orElse-sentinel) #841

Description

@cuioss-oliver

Problem

The junit-core skill's assertion guidance (pm-dev-java/skills/junit-core/standards/testing-junit-core.md, "JUnit 5 Assertion Features" at :37) covers assertInstanceOf, assertAll, assertThrows, assertDoesNotThrow, assertTimeout — but says nothing about asserting Optional return values. This leaves a common anti-pattern unaddressed.

An Optional assertion actually verifies two distinct properties:

  1. whether a value is present, and
  2. the contained value.

Collapsing absence into a sentinel conflates the two:

assertEquals(204, result.getHttpStatus().orElse(-1));

When the Optional is empty, this fails as expected: <204> but was: <-1> — which reads as a present-but-wrong value and hides the real cause (absence). It violates the skill's own stated principle that failure messages must "describe what should have happened" (testing-junit-core.md:59).

Observed at 10 sites in cuioss/cui-http (adapter integration tests) during a post-merge review; likely present across other repos.

Proposed guidance (new subsection in standards/testing-junit-core.md)

Because an Optional assertion covers presence and value, pick the idiom by which property the test asserts:

  • Both matter (default case) — assert the whole Optional:

    assertEquals(Optional.of(204), result.getHttpStatus());

    Single assertion; the failure message distinguishes Optional.empty from a wrong value.

  • Presence is itself a first-class, separately-reported assertion — assert both explicitly (optionally via assertAll):

    var status = result.getHttpStatus();
    assertTrue(status.isPresent(), "status must be present");
    assertEquals(204, status.get(), "status should be 204");
  • Presence is an assumed precondition, only the value is under testorElseThrow():

    assertEquals(204, result.getHttpStatus().orElseThrow());

Anti-pattern: assertEquals(expected, opt.orElse(sentinel)) — masks absence as a wrong value.

Constraints / cross-checks

  • JUnit-5-only. No AssertJ hasValue/contains — the skill already mandates "never introduce libraries without asking" (testing-junit-core.md:7).
  • Do not imply "always orElseThrow". ext-triage-java/standards/pr-comment-disposition.md:41 (correctly) argues against forcing orElseThrow on production Optional.get() calls whose presence is already asserted. That is the production case; this guidance is about test assertions, where the throw is the failure signal. The two must stay consistent — the new subsection is scoped to test assertions.

Source

Raised during the cui-http post-merge adversarial review (PRs #73#78), finding F-02. The repo-level fix in cui-http should apply this guidance once landed here.

Activity

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

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions