Skip to content

[2.x] test(tags): fetch the created tag by slug rather than by row order - #4963

Merged
imorland merged 1 commit into
2.xfrom
im/tags-create-test-deterministic
Aug 22, 2026
Merged

[2.x] test(tags): fetch the created tag by slug rather than by row order#4963
imorland merged 1 commit into
2.xfrom
im/tags-create-test-deterministic

Conversation

@imorland

@imorland imorland commented Aug 21, 2026

Copy link
Copy Markdown
Member

Surfaced by the reworked CI matrix in #4959, which adds PostgreSQL 18. The job failed on:

1) Flarum\Tags\Tests\integration\api\tags\CreateTest::admin_can_create_tag
Failed asserting that two strings are equal.
-'Dev Blog'
+'General'

Changes proposed in this pull request:

The test creates a tag over the API and then reads it back with:

$tag = Tag::all()->last();

Tag has no ordering scope, so that is a select with no order by, and which row comes back last is entirely up to the database. When it is not the row just inserted, every assertion after it compares one of the seeded tags instead.

It is flaky rather than consistently broken, which is worth being precise about: the PostgreSQL 18 job passed on two earlier runs of #4959 and failed on the third, and in that run the prefixed and unprefixed PostgreSQL 18 jobs disagreed with each other on identical code. PostgreSQL makes it more visible than MySQL for two reasons — synchronize_seqscans is enabled by default, so a sequential scan may begin part-way through the table to share work with a concurrent scan, and an UPDATE writes the new tuple version at the end of the heap, moving rows. Neither is specific to version 18, and nothing stops this firing on MySQL.

The test means "the tag I just created", so it now says so:

$tag = Tag::firstWhere('slug', 'dev-blog');
$this->assertNotNull($tag);

Same class of problem as #4962, which fixed it in the listing code — this is the one remaining instance in a test. Tag::all()->last() was the only occurrence of the pattern anywhere in the monorepo.

This is not a PostgreSQL 18 bug and not a regression. The test has never been sound; it has simply been getting away with it.

Reviewers should focus on:

  • Whether firstWhere is specific enough. The slug is unique in this fixture and the tag is created by the request under test, so there is exactly one candidate. Ordering by id descending would also work but would keep the test coupled to insertion order for no benefit.

Necessity

  • Has the problem that is being solved here been clearly explained?
  • If applicable, have various options for solving this problem been considered? — ordering the query explicitly was the alternative; fetching the record by the value under test expresses the intent better.
  • For core PRs, does this need to be in core, or could it be in an extension? — bundled extension test only.
  • Are we willing to maintain this for years / potentially forever?

Confirmed

  • Frontend changes: tested on a local Flarum installation — no frontend changes.
  • Frontend changes: tests are green — no frontend changes.
  • Frontend changes: tests have been added — no frontend changes.
  • Backend changes: tests are green — CI on this branch; PostgreSQL 18 is only covered once [2.x] ci: rebuild the backend matrix around declared database support #4959 lands, so that is where this is really proven.
  • Backend changes: tests have been added — this corrects an existing test rather than adding one.
  • Where applicable, changes are suitable for all supported database drivers — the point of the change is that it no longer depends on the driver.
  • Core developer confirmed locally this works as intended.
  • The description above is written by me and describes what this pull request actually does.

@imorland
imorland requested a review from a team as a code owner August 21, 2026 23:58
@imorland imorland added this to the 2.0.0-rc.6 milestone Aug 22, 2026
@imorland
imorland merged commit 10d6a6f into 2.x Aug 22, 2026
25 checks passed
@imorland
imorland deleted the im/tags-create-test-deterministic branch August 22, 2026 00:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant