Skip to content

test: replace checkbox assertions with real assertions or DoesNotPerformAssertions#60742

Merged
miaulalala merged 1 commit into
masterfrom
test/noid/remove-checkbox-tests
May 27, 2026
Merged

test: replace checkbox assertions with real assertions or DoesNotPerformAssertions#60742
miaulalala merged 1 commit into
masterfrom
test/noid/remove-checkbox-tests

Conversation

@miaulalala

@miaulalala miaulalala commented May 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Seven test methods across five files used `assertTrue(true)` or `addToAssertionCount(1)` purely to silence PHPUnit's "no assertions" warning, providing no actual verification of behaviour.

  • ClearFrontendCachesTest::testRun — already had mock expectations (assert-once for `clear`, `resetCache`, `createDistributed`); the trailing `assertTrue(true)` was pure noise
  • BrokerTest::testNewConversationOptions — `newConversationOptions()` returns an `IConversationOptions` object; now asserts the return type rather than nothing
  • SharedQueryBuilderTest (3× validate tests) — each test verifies that `validate()` does not throw for a legitimately valid query; replaced `assertTrue(true)` with `#[DoesNotPerformAssertions]`, which makes the intent explicit without pretending there's an assertion
  • PublicShareMiddlewareTest (3× no-op path tests) — same pattern: tests verify no exception is thrown when middleware hits a non-exceptional path; replaced `assertTrue(true)` with `#[DoesNotPerformAssertions]`
  • InMemoryFileTest::testDelete — `delete()` is documented as a no-op for in-memory files; replaced the workaround comment + `assertTrue(true)` with `#[DoesNotPerformAssertions]`

Why

Tests that assert `assertTrue(true)` give a false sense of coverage. When a method's behaviour changes in a breaking way, these tests stay green and provide no signal. `#[DoesNotPerformAssertions]` is honest: it explicitly says "this test passes if it doesn't throw", which is at least a truthful contract.

Test plan

  • All five modified test classes pass locally with `NOCOVERAGE=1 ./autotest.sh sqlite`
  • No new PHPUnit warnings introduced

🤖 Generated with Claude Code

@miaulalala miaulalala self-assigned this May 26, 2026
@miaulalala miaulalala added 3. to review Waiting for reviews tests Related to tests CI labels May 26, 2026
@miaulalala miaulalala added this to the Nextcloud 35 milestone May 26, 2026
@miaulalala miaulalala marked this pull request as ready for review May 26, 2026 19:34
@miaulalala miaulalala requested a review from a team as a code owner May 26, 2026 19:34
@miaulalala miaulalala requested review from ArtificialOwl, artonge, leftybournes and provokateurin and removed request for a team May 26, 2026 19:34
miaulalala added a commit that referenced this pull request May 26, 2026
… (middleware)

Replace `addToAssertionCount(1)` placeholders in AppFramework middleware
tests that only verify no exception is thrown.

For `SameSiteCookieMiddlewareTest`, two tests already have
`expects(self::once())` mock expectations as their real assertion — those
just have the redundant placeholder removed. The third test has no
expectations and gets `#[DoesNotPerformAssertions]`.

For `SecurityMiddlewareTest`, `testIsSubAdminCheck` and
`testIsSubAdminAndAdminCheck` get `#[DoesNotPerformAssertions]`.
`testRestrictedAppLoggedInPublicPage` and `testRestrictedAppNotLoggedInPublicPage`
have `->with()` argument constraints that count as assertions in PHPUnit 11,
so the redundant placeholder is simply removed.

Part of a broader effort to eliminate checkbox tests (see also #60742, #60747).

Signed-off-by: Anna Larch <anna@nextcloud.com>
AI-Assisted-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@miaulalala miaulalala force-pushed the test/noid/remove-checkbox-tests branch from 8bc864e to 45d2121 Compare May 27, 2026 07:46
@miaulalala miaulalala requested a review from a team as a code owner May 27, 2026 07:46
@miaulalala miaulalala requested review from nfebe, sorbaugh and susnux and removed request for a team May 27, 2026 07:46
@miaulalala

Copy link
Copy Markdown
Contributor Author

/backport to stable34

@miaulalala miaulalala force-pushed the test/noid/remove-checkbox-tests branch from 45d2121 to 30abe60 Compare May 27, 2026 09:19
@miaulalala miaulalala requested a review from blizzz May 27, 2026 09:40
susnux
susnux previously requested changes May 27, 2026

@susnux susnux left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

your commit contains many unrelated translation files?

Replace assertTrue(true), addToAssertionCount(1) and delete-without-assert
patterns with meaningful assertions or proper test removal.

Signed-off-by: Anna Larch <anna@nextcloud.com>
AI-Assisted-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@miaulalala miaulalala force-pushed the test/noid/remove-checkbox-tests branch from 30abe60 to 18c5c07 Compare May 27, 2026 10:12
@miaulalala miaulalala requested a review from susnux May 27, 2026 10:13
@miaulalala miaulalala merged commit f67b908 into master May 27, 2026
220 of 229 checks passed
@miaulalala miaulalala deleted the test/noid/remove-checkbox-tests branch May 27, 2026 17:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review Waiting for reviews CI tests Related to tests

4 participants