Repository navigation
Bug 2020752 - [PHABBUGZ] Improve testing of phabricator and bmo integration - #2790
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Fixture ID collisions and unrealistic attachment responses undermine test isolation and integration coverage.
2 open issues
What changed in this PR
Expands PhabBugz integration testing against BMO using an in-process fake Phabricator Conduit service.
Changes:
- Adds reusable fixtures, request simulation and test-environment helpers.
- Covers models, synchronization, permissions, approval flags and failure handling.
- Seeds product-scoped and disabled uplift approval flags.
| File | Description |
|---|---|
| scripts/generate_bmo_data.pl | Seeds uplift approval flag types. |
| extensions/PhabBugz/t/bmo/webservice.t | Tests endpoints and permissions. |
| extensions/PhabBugz/t/bmo/util.t | Tests attachments and reviewer helpers. |
| extensions/PhabBugz/t/bmo/model-user.t | Tests account lookup and synchronization. |
| extensions/PhabBugz/t/bmo/model-revision.t | Tests revision models and privacy helpers. |
| extensions/PhabBugz/t/bmo/model-repository-comment.t | Tests repository and comment models. |
| extensions/PhabBugz/t/bmo/model-project.t | Tests project lookup and updates. |
| extensions/PhabBugz/t/bmo/model-policy.t | Tests custom policy handling. |
| extensions/PhabBugz/t/bmo/lib/PhabTest/Fixtures.pm | Builds Conduit fixtures. |
| extensions/PhabBugz/t/bmo/lib/PhabTest/Env.pm | Provides setup and teardown helpers. |
| extensions/PhabBugz/t/bmo/lib/PhabTest/Conduit.pm | Simulates and records Conduit requests. |
| extensions/PhabBugz/t/bmo/feed-users.t | Tests new-user synchronization. |
| extensions/PhabBugz/t/bmo/feed-stories.t | Tests revision synchronization and failures. |
| extensions/PhabBugz/t/bmo/feed-run-query.t | Tests daemon error handling and cleanup. |
| extensions/PhabBugz/t/bmo/feed-groups.t | Tests group membership synchronization. |
| extensions/PhabBugz/t/bmo/conduit-request.t | Tests transport encoding and errors. |
| extensions/PhabBugz/t/bmo/approval-flags.t | Tests uplift approval flag behavior. |
Review effort: Balanced
cgsheeh
left a comment
cgsheeh
left a comment
There was a problem hiding this comment.
Left a few comments. As this PR is just adding new tests, I'm fine with landing as-is and we can iterate. :)
|
|
||
| sub reply_feed_query_id { | ||
| my ($self, $params) = @_; | ||
| my $after = $params->{after} // 0; |
There was a problem hiding this comment.
The real feed.query_id errors when after isn't a valid story id (that's why new_stories has the retry loop), but this fake returns everything above after. Does that let tests like the after => 1 and 7_000_000 cursor cases pass where production would retry or throw? Should the fake return cursor_invalid when after doesn't match a story (other than the initial cursor)?
| $conduit->queue('feed.query_id' => cursor_invalid($cursor)); | ||
| ok(!run_feed(), 'feed_query recovers from an invalid cursor'); | ||
| is_deeply( | ||
| [map { $_->{after} } $conduit->calls('feed.query_id')], |
There was a problem hiding this comment.
With gap => 2 there's no story at $cursor + 1, so by real Conduit's behaviour wouldn't this need a third retry? Is this assertion only passing because the fake ignores unknown after ids?
|
|
||
| sub reply_feed_query_id { | ||
| my ($self, $params) = @_; | ||
| my $after = $params->{after} // 0; |
There was a problem hiding this comment.
This ignores page_size, unlike _page and user.search. Should it honour the limit so we can test the "one page per run, rest on the next run" behaviour for stories like feed-users.t does for users?
| view => $args{view_policy} // 'public', | ||
| edit => $args{edit_policy} // 'users', | ||
| }, | ||
| 'bugzilla.bug-id' => $args{bug_id} // '', |
There was a problem hiding this comment.
I think Conduit returns null for an unset bugzilla.bug-id custom field. Should this default to undef so the "missing bug id becomes an empty string" test in model-revision.t actually exercises the null → '' conversion in Revision::new_from_query?
| # Only NEW and UNCONFIRMED bugs move to ASSIGNED; BMO files bugs as | ||
| # CONFIRMED, which is left alone. | ||
| is( | ||
| $bug->bug_status, |
There was a problem hiding this comment.
Since test bugs are filed as CONFIRMED, this only ever checks "status unchanged". Should we also file a NEW/UNCONFIRMED bug so the ASSIGNED transition in set_bug_assignee gets covered?
| ok(!run_feed(), 'story after a group was added to the bug'); | ||
| my ($recreate) = $conduit->calls('policy.create'); | ||
| is_deeply( | ||
| [sort @{$recreate->{policy}[-1]{value}}], |
There was a problem hiding this comment.
If policy.create stops being called, $recreate is undef and this dereference kills the whole file. Should we assert ok($recreate, ...) first?
| 'form not copied again' | ||
| ); | ||
| is(scalar(phab_attachments($bug->id)), 1, 'normal sync ran'); | ||
|
|
There was a problem hiding this comment.
This cleanup only runs on the success path. Should it go through $env->on_teardown like the other cleanups?
| use Digest::SHA qw(sha1_hex); | ||
| use Storable qw(dclone); | ||
| use Test::More; | ||
| use Try::Tiny; |
There was a problem hiding this comment.
Is Try::Tiny used in this file?
| use Bugzilla; | ||
| BEGIN { Bugzilla->extensions } | ||
|
|
||
| use Storable qw(dclone); |
| grant_group => '', | ||
| target_type => 'a', | ||
| cc_list => '', | ||
| inclusions => ['Firefox:General'], |
There was a problem hiding this comment.
These flags will now show up on attachments for every Firefox :: General bug in dev/test DBs. Given Selenium wasn't run, is it worth running it to make sure nothing there depends on the attachment flag list?
Xzzz
left a comment
Xzzz
left a comment
There was a problem hiding this comment.
Thanks! Nothing to add to Connor's points. lgtm once CI runs successfully
Add a mocked PhabBugz test suite and seed approval-mozilla-* flag types
Summary
Adds a test suite for the Bugzilla/Phabricator integration that runs without
a Phabricator server. Every Conduit call goes to an in-process fake that
answers with the response shapes from the Conduit API docs. The tests live in
extensions/PhabBugz/t/bmo/, so CI's existingtest_bmojob picks them upand runs them against the real MySQL database. No CI changes are needed.
Also seeds the
approval-mozilla-*flag types ingenerate_bmo_data.pl, soset_attachment_approval_flagscan be tested. This includes the commonproduction failure where the flag is not enabled for the bug's
product/component.
No production code is changed.
Changes
Test helpers (
extensions/PhabBugz/t/bmo/lib/PhabTest/)Conduit.pm: a fake Conduit server. It overridesMojo::UserAgent::postand records every call. It answers from an in-memory directory of users,
projects, revisions, repositories, policies, feed stories, transactions and
edges, and applies
*.editcalls to that directory. Tests can queuefailures: Conduit errors, HTTP errors, non-JSON bodies, network errors and
an invalid feed cursor. A call to a method the fake doesn't handle fails the
test.
Fixtures.pm: builders for complete Conduit objects.Env.pm: per-file setup and teardown for the shared database. It createsfixtures with unique names, restores the
phabbugzcursors, deactivatestest groups, and only ever sets PhabBugz params in memory.
Tests (13 files)
conduit-request.t: howrequest()encodes calls, and how it handlesmissing config, HTTP 4xx/5xx, bad or truncated JSON, Conduit errors (with
and without
no_die), network failures and a missingresult.model-{user,project,policy,revision,repository-comment}.t: lookups,paging, lazy builders, the exact transactions each mutator sends, privacy
helpers, and errors from Phabricator.
model-user.talso covers syncing adisabled BMO account to Phabricator.
util.t: bug role PHIDs, revision attachments, auto-assignment, reviewerrotation and intermittent reviewers.
webservice.t:bug_revisions,userand the phab-bot permission checks.approval-flags.t:?on new revisions,+only from release managers,Xhandlingdirectly and through the feed
feed-stories.t,feed-users.t,feed-groups.t: the daemon's periodicloops, covering:
feed-run-query.t: the daemon wrapper logs errors, clears the watchdogalarm and keeps running.
Seed data (
scripts/generate_bmo_data.pl)approval-mozilla-{beta,release,esr140}: enabled only forFirefox :: General, so
bugzilla.mozilla.organd Firefox :: Installerreproduce the "flag not enabled" case.
approval-mozilla-esr115: seeded but disabled (is_active = 0).is_active, defaulting to 1. The existingflag types are unchanged.
Issues found (left as TODO tests, not fixed here)
feed_queryhas noper-story error handling, so the cursor never moves past the failing story.
The most likely trigger is an approval flag that isn't enabled for the
bug's product/component:
set_flagsthrowsinvalid_flag_associationandthe whole story is rolled back.
Project::updatenever sends thejoinpolicy.revision/comments.html.tmpldoesn't loadterms. Theinvalid-bug comment posted to Phabricator reads "invalid ID", with the
word "bug" missing.
Also noted, but tested as current behavior rather than as TODOs:
Policy->create([])sends a fallback rule without arulekey.needs-revisioncreates an approval-flag without arelease manager check.
Testing
test_bmo: 40 files, 1,786 tests pass. The PhabBugz suite was also runtwice in one database session to check that it cleans up after itself.
test_webservices: 45 files, 3,907 tests pass with the new seed data.test_sanity: the critic, goodperl and whitespace checks pass.🤖 Generated with Claude Code