Conversation
smarcet
force-pushed
the
hotfix/unit-test-fixes
branch
from
68eba0f to
d57536e
Compare
…mmitSponsorApiTest chore(unit-test): fix OAuth2SummitSponsorApiTest chore(unit-test): fix OAuth2AttendeesApiTest
chore(ci): add to OAuth2BookableRoomAttributeTypesApiTest
chore(ci): add OAuth2ElectionsApiController
chore(ci): add OAuth2MembersApiTest
chore(unit-test): fix OAuth2OAuth2SponsorshipTypeApiTest
chore(unit-test): fix OAuth2OrganizationsApiTest
chore(ci): add OAuth2PresentationApiTest
chore(ci): add OAuth2SummitPromoCodesApiTest chore(unit-test): fix OAuth2PromoCodesApiTest and OAuth2SummitPromoCodesApiTest
chore(ci): add OAuth2SelectionPlansApiTest
chore(ci): add OAuth2SummitSpeakersApiTest
chore(ci): add OAuth2SummitBadgesApiTest
chore(ci): add OAuth2TracksApiTest
chore(ci): add OAuth2TrackTagGroupsApiTest
chore(ci): add OAuth2SummitTracksApiTest and OAuth2TagsApiTest
chore(ci): add several unit tests to GA
* OAuth2SummitTicketTypesApiTest * OAuth2SummitTicketsApiTest * OAuth2SummitTaxTypeApiTest * OAuth2TrackGroupsApiTest * OAuth2TrackQuestionsTemplateTest
OAuth2SummitBadgeTypeApiTest — 11 tests. OAuth2SummitBadgeViewTypeApiTest — 6 tests. OAuth2SummitDocumentsApiControllerTest — 12 tests. OAuth2SummitEmailEventFlowApiControllerTest — 4 tests. OAuth2SummitEventsBulkActionsTest — 2 tests. OAuth2SummitLocationsApiTest — 69 tests.
smarcet
force-pushed
the
hotfix/unit-test-fixes
branch
from
eb5d431 to
1d46bfd
Compare
chore(ci): add OAuth2SummitTrackChairsRankingApiTest to GA pipeline
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughBroad refactor of the test suite to use fixture-driven setup (InsertSummitTestData), replace parameterized tests with context-based methods, add many new/renamed tests and lifecycle hooks; minor production edits: trait addition, method type-hint, SQL GROUP BY, route middleware, Doctrine clear before seeding, and removal of per-item event dispatch. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
tests/OAuth2SummitSponsorshipTypeApiControllerTest.php (1)
465-477:⚠️ Potential issue | 🟡 MinorBug: DELETE action response not captured.
The DELETE action result is not assigned to
$response, so line 476 retrieves content from the previous POST response (line 448). WhileassertResponseStatus(204)likely checks the framework's internal last-response state and may pass, the$contentvariable holds stale data.🐛 Proposed fix to capture DELETE response
- $this->action( + $response = $this->action( "DELETE", "OAuth2SummitSponsorshipTypeApiController@removeBadgeImage", $params, [], [], [ ], $headers ); - $content = $response->getContent(); $this->assertResponseStatus(204); }Note:
$contentis unused after the DELETE, so the assignment can be removed entirely.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitSponsorshipTypeApiControllerTest.php` around lines 465 - 477, The DELETE call to OAuth2SummitSponsorshipTypeApiController@removeBadgeImage is not assigned to $response so $content = $response->getContent() reads a stale response; fix by assigning the result of $this->action("DELETE", "OAuth2SummitSponsorshipTypeApiController@removeBadgeImage", $params, ..., $headers) back into $response (or remove the $content = $response->getContent() line entirely if $content is unused) and keep the subsequent $this->assertResponseStatus(204) unchanged.database/seeders/ConfigSeeder.php (1)
27-55:⚠️ Potential issue | 🟠 MajorRollback and rethrow seeding failures.
This block still swallows exceptions. If anything fails after Line 35, the connection can remain inside a transaction; if a later seeder fails after Line 48, the command still reports success with partial config data. Please roll back when a transaction is active and rethrow.
Suggested fix
public function run() { Model::unguard(); + $connection = null; try { // clear all $em = Registry::getManager(ResourceServerEntity::EntityManager); $em->clear(); $connection = $em->getConnection(); $connection->beginTransaction(); $statements = [ 'DELETE FROM endpoint_api_scopes', 'DELETE FROM endpoint_api_authz_groups', 'DELETE FROM api_endpoints;', 'DELETE FROM api_scopes;', 'DELETE FROM apis;', ]; foreach ($statements as $sql) { $connection->executeStatement($sql); } $connection->commit(); $this->call(ApiSeeder::class); $this->call(ApiScopesSeeder::class); $this->call(ApiEndpointsSeeder::class); } - catch (\Exception $ex){ + catch (\Throwable $ex){ + if ($connection !== null && $connection->isTransactionActive()) { + $connection->rollBack(); + } Log::error($ex); + throw $ex; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@database/seeders/ConfigSeeder.php` around lines 27 - 55, The run() method currently begins a DB transaction, commits it before calling ApiSeeder/ApiScopesSeeder/ApiEndpointsSeeder, and swallows exceptions; change the flow so the transaction encompasses the delete statements plus the subsequent seeder calls (move $connection->commit() to after the $this->call(...) lines), and wrap the transactional work in try/catch/finally: on catch, if $connection->isTransactionActive() call $connection->rollBack(), log the exception (Log::error($ex)), and then rethrow the exception so the command fails; ensure any finally block cleans up or releases resources but does not suppress the rethrown exception.tests/OAuth2TrackQuestionsTemplateTest.php (1)
40-63:⚠️ Potential issue | 🟡 MinorAssert the filter result, not just that the payload parses.
A
200with unrelated templates would still pass here. Add at least one assertion that the returned items actually matchTrackTextBoxQuestionTemplate, otherwise this test will miss regressions in theclass_namefilter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2TrackQuestionsTemplateTest.php` around lines 40 - 63, The test testGetTrackQuestionTemplateByClassName currently only decodes the response and checks status; update it to assert the filter actually worked by inspecting the decoded response (track_question_templates) returned from OAuth2TrackQuestionsTemplateApiController@getTrackQuestionTemplates and verifying at least one item exists and that each item's class_name (or equivalent property) equals "TrackTextBoxQuestionTemplate"; if the response shape nests items (e.g., data or items), adapt to that shape and assert the class_name on those elements rather than just checking for non-null JSON.tests/OAuth2AttendeesApiTest.php (1)
229-234:⚠️ Potential issue | 🟡 MinorBug:
testAddAttendee()is called with an argument but accepts none.Line 230 calls
$this->testAddAttendee(3)buttestAddAttendee()at line 196 has no parameters. This will work in PHP (extra arguments are ignored), but it appears to be leftover code from when the method accepted amember_idparameter.🐛 Suggested fix
public function testDeleteAttendee(){ - $attendee = $this->testAddAttendee(3); + $attendee = $this->testAddAttendee();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2AttendeesApiTest.php` around lines 229 - 234, The call in testDeleteAttendee passes an unused argument to testAddAttendee (testAddAttendee(3)) while testAddAttendee has no parameters; remove the extraneous argument from the call in testDeleteAttendee (change testAddAttendee(3) to testAddAttendee()) or alternatively add a parameter to testAddAttendee if the test legitimately needs a member_id—update the invocation and/or the testAddAttendee signature accordingly to keep them consistent (refer to testDeleteAttendee and testAddAttendee).tests/OAuth2SelectionPlansApiTest.php (1)
865-904:⚠️ Potential issue | 🟡 MinorAssert the action-type association/order actually changed.
Both tests only prove the endpoint returned
201and some JSON. They still pass if the controller ignores the requestedtype_id/order. Please assert the returned action type matches the one you added, and that the reorder case really moved it to the requested position.Also applies to: 906-942
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SelectionPlansApiTest.php` around lines 865 - 904, The test testAddPresentationActionTypeToSelectionPlan currently only checks for a 201 and non-null JSON; update it to verify the created association and order by decoding the response and asserting that the returned action-type id equals $new_action_type->getId() and that the returned "order" equals 1 (or the expected position). Similarly, for the reorder test referenced (lines 906-942), decode the response and assert the specific action type ids and their "order" fields reflect the requested move (e.g., that the moved type now has the requested order and others were reindexed accordingly); use the controller response body (entity) fields rather than only HTTP status to validate the change. Ensure you reference the test methods testAddPresentationActionTypeToSelectionPlan and the reorder test method and check the "type_id" / "id" and "order" properties in the JSON payload returned by addAllowedPresentationActionType.
🟡 Minor comments (12)
tests/OAuth2SummitSponsorshipTypeApiControllerTest.php-47-59 (1)
47-59:⚠️ Potential issue | 🟡 MinorMissing cleanup of
$testSponsorshipTypein tearDown.The
SponsorshipTypeentity is created and persisted insetUp(), but it's not explicitly removed intearDown(). IfclearSummitTestData()doesn't handle this entity type, it could leave orphaned records between test runs.🧹 Proposed fix to add cleanup
protected function tearDown(): void { + if (self::$testSponsorshipType !== null) { + self::$em->remove(self::$testSponsorshipType); + self::$em->flush(); + self::$testSponsorshipType = null; + } self::clearSummitTestData(); parent::tearDown(); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitSponsorshipTypeApiControllerTest.php` around lines 47 - 59, setUp() creates and persists self::$testSponsorshipType but tearDown() doesn't remove it; update tearDown() to explicitly delete the created SponsorshipType (self::$testSponsorshipType) and flush the entity manager to avoid orphaned records between tests—locate the test class's tearDown() method and add logic to check for non-null self::$testSponsorshipType, call $this->em->remove(self::$testSponsorshipType) and $this->em->flush(), then null out self::$testSponsorshipType so subsequent runs are clean; ensure this cleanup runs before or after clearSummitTestData() depending on dependency order.tests/OAuth2PresentationApiTest.php-1454-1454 (1)
1454-1454:⚠️ Potential issue | 🟡 MinorIncorrect HTTP status assertion for GET request.
The
testGetPresentationSubmissiontest asserts a201status code for a GET request, but GET endpoints should return200 OKon success, not201 Created.Proposed fix
- $this->assertResponseStatus(201); + $this->assertResponseStatus(200);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2PresentationApiTest.php` at line 1454, The test testGetPresentationSubmission incorrectly asserts a 201 status for a GET request; update the assertion in testGetPresentationSubmission to expect 200 instead of 201 by changing the call to assertResponseStatus on the GET response (the assertResponseStatus(...) invocation used in testGetPresentationSubmission) so the test checks for HTTP 200 OK.tests/OAuth2SummitTracksApiTest.php-323-344 (1)
323-344:⚠️ Potential issue | 🟡 MinorRemove unused
$responsevariable at line 333.The test correctly expects a 204 response since the track exists (created in setUp) and
clearIcon()is idempotent. However, the$responsevariable is assigned but never used.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitTracksApiTest.php` around lines 323 - 344, In testDeleteTrackIcon, remove the unused local variable $response: instead of assigning the result of $this->action(...) to $response, invoke $this->action(...) without capturing the return value (or simply unset $response) so the test no longer defines an unused variable; keep the call to OAuth2SummitTracksApiController@deleteTrackIcon and the existing assertions unchanged.tests/OAuth2SummitDocumentsApiControllerTest.php-278-301 (1)
278-301:⚠️ Potential issue | 🟡 MinorAssert the association is actually removed, not just the status code.
Both new DELETE tests pass even if the handler returns
201without unlinking anything. Reload the affected entity after the request and assert that the document/event type is no longer present in the relation.Also applies to: 328-351
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitDocumentsApiControllerTest.php` around lines 278 - 301, After calling OAuth2SummitDocumentsApiController::removeEventType in testRemoveEventTypeFromDocument, reload the affected document and assert the event type association has been removed instead of only checking the response code; for example, re-fetch the document from self::$summit (using self::$summit->getSummitDocuments() or the repository/ORM refresh) and assert that the event type from self::$summit->getEventTypes()[0] is no longer present (via hasEventType, contains, or assertFalse/assertNotContains). Apply the same change to the other DELETE test block referenced (lines ~328-351) so both tests verify the relation was actually unlinked, not just that a 201 was returned.tests/OAuth2AttendeesApiTest.php-407-414 (1)
407-414:⚠️ Potential issue | 🟡 MinorSame null check concern for testReassignAttendeeTicket.
Similar to
testDeleteAttendeeTicket, add a guard assertion before accessing the first ticket.🛡️ Suggested fix
public function testReassignAttendeeTicket(){ $attendee = self::$summit->getAttendeeByMember(self::$defaultMember); $this->assertNotNull($attendee); + $ticket = $attendee->getTickets()->first(); + $this->assertNotFalse($ticket, 'Attendee must have at least one ticket'); $params = [ 'id' => self::$summit->getId(), 'attendee_id' => $attendee->getId(), - 'ticket_id' => $attendee->getTickets()->first()->getId(), + 'ticket_id' => $ticket->getId(), ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2AttendeesApiTest.php` around lines 407 - 414, In testReassignAttendeeTicket, add a guard assertion to ensure the attendee has at least one ticket before calling $attendee->getTickets()->first(); for example assertNotNull($attendee->getTickets()->first()) or assertGreaterThan(0, $attendee->getTickets()->count()) right after retrieving $attendee so the subsequent use of attendee->getTickets()->first() is safe and won't cause a null access.tests/OAuth2AttendeesApiTest.php-381-387 (1)
381-387:⚠️ Potential issue | 🟡 MinorAdd null check before accessing ticket from collection.
getTickets()->first()returnsfalseif the collection is empty, which would cause->getId()to fail. Consider adding a guard assertion.🛡️ Suggested fix
public function testDeleteAttendeeTicket(){ $attendee = self::$summit->getAttendeeByMember(self::$defaultMember); + $this->assertNotNull($attendee); + $ticket = $attendee->getTickets()->first(); + $this->assertNotFalse($ticket, 'Attendee must have at least one ticket'); $params = [ 'id' => self::$summit->getId(), 'attendee_id' => $attendee->getId(), - 'ticket_id' => $attendee->getTickets()->first()->getId() + 'ticket_id' => $ticket->getId() ];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2AttendeesApiTest.php` around lines 381 - 387, In testDeleteAttendeeTicket, guard against an empty ticket collection before calling ->getId() by first retrieving the ticket into a variable (e.g., $ticket = $attendee->getTickets()->first()) and asserting it is not false/null (or failing the test with a clear message) before using $ticket->getId(); update the params construction to use the guarded $ticket variable and ensure the test fails fast with a descriptive assertion if no ticket exists.tests/OAuth2SummitBadgeTypeApiTest.php-206-229 (1)
206-229:⚠️ Potential issue | 🟡 MinorRemove the unused
$contentvariable fetches from test methods.The test methods
testRemoveAccessLevelFromBadgeType(line 227),testRemoveFeatureFromBadgeType, andtestRemoveViewTypeFromBadgeTypeall fetch$response->getContent()but never use it. Either remove these lines or add assertions on the content.HTTP 201 is the correct status code for these endpoints—the API intentionally returns the updated badge type resource (not an empty response), making 201 appropriate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitBadgeTypeApiTest.php` around lines 206 - 229, Remove the unused $content = $response->getContent() calls in the test methods (testRemoveAccessLevelFromBadgeType, testRemoveFeatureFromBadgeType, testRemoveViewTypeFromBadgeType) or replace them with meaningful assertions against the returned payload; locate the calls in those methods and either delete the $content assignment or add assertions examining the response content (e.g., decode JSON and assert expected fields on the updated badge type) while keeping the existing assertResponseStatus(201) checks intact.tests/OAuth2SummitPromoCodesApiTest.php-219-223 (1)
219-223:⚠️ Potential issue | 🟡 MinorAssert the attached speaker is actually returned.
The setup added on Lines 219-223 and 247-265 now makes the expected speaker deterministic, but the tests still stop at
200plusjson_decode() !== null. An empty payload would still pass here, so this won’t catch regressions in the speaker association endpoints.Also applies to: 247-265
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitPromoCodesApiTest.php` around lines 219 - 223, The response check currently only verifies HTTP 200 and non-null JSON; update the test that calls testAddPromoCodeSpeaker() (referencing $created and promo_code_id) to decode the JSON response body and assert the attached speaker is present and matches the created speaker (e.g., compare the returned speaker's id or unique attribute to $created->id/$created->email), and apply the same stronger assertion in the other test block covering lines 247-265 so an empty payload will fail.tests/OAuth2SummitTrackChairsApiTest.php-368-422 (1)
368-422:⚠️ Potential issue | 🟡 MinorVerify the row is actually gone after
DELETE.This test stops at
204, so a no-op delete or missing flush/transaction would still pass. Follow the delete with a GET/list assertion and expect the API contract’s post-condition (404, or the collection total dropping by one).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitTrackChairsApiTest.php` around lines 368 - 422, In testDeleteTrackChair, after asserting the 204 from OAuth2SummitTrackChairsApiController@delete, perform a follow-up request to verify the row was actually removed: either call the GET/show endpoint for that track_chair_id (using action(...) with method "GET" against the controller's show/get method) and assert a 404, or call the list endpoint (e.g. the controller's list method) and assert the collection total decreased by one; use the same headers pattern, decode the response JSON and assert the expected status/body to ensure the delete had effect.tests/OAuth2SelectionPlansApiTest.php-1026-1056 (1)
1026-1056:⚠️ Potential issue | 🟡 MinorThis does not isolate the allowed-member code path.
setUp()already authenticates a track chair and attaches them to the summit, sopage->total >= 1can pass even ifaddAllowedMember()has no effect. Use a member/plan combination that is only visible throughallowed_members, or this test will keep producing false positives.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SelectionPlansApiTest.php` around lines 1026 - 1056, The test testGetMySelectionPlans currently relies on setUp() which authenticates a track chair so the GET path may return plans via chair permissions, producing false positives; change the test to isolate the allowed-member path by creating or fetching a selection plan that the authenticated user would not otherwise see (e.g., create a new SelectionPlan instance or use a plan not associated with the user's chair role), call addAllowedMember(self::$member->getEmail()) on that specific plan, persist and flush the new plan (use symbols default_selection_plan only if you first ensure it isn't visible via chair permissions), then invoke OAuth2SummitSelectionPlansApiController@getMySelectionPlans and assert the response total equals 1 (or specifically that the newly-created plan is present) so the test only passes when allowed_members is honored.tests/OAuth2SummitSpeakersApiTest.php-1496-1518 (1)
1496-1518:⚠️ Potential issue | 🟡 MinorAssert all fields sent to
updateMySpeaker.This request updates
title,first_name,last_name, andbio, but the test only verifiestitle. A partial regression in the other fields would still pass here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitSpeakersApiTest.php` around lines 1496 - 1518, The test updates multiple fields in $data but only asserts the title; modify the OAuth2SummitSpeakersApiTest updateMySpeaker test to assert that $speaker->first_name, $speaker->last_name, and $speaker->bio match the values in $data (e.g., 'UpdatedFirst', 'UpdatedLast', 'Updated bio text') after calling the updateMySpeaker endpoint; locate the assertions around the existing $speaker variable and add equality checks for first_name, last_name, and bio to prevent partial-regression slips.tests/OAuth2SummitSpeakersApiTest.php-1833-1839 (1)
1833-1839:⚠️ Potential issue | 🟡 MinorAssert the edit-permission state change, not just
200.A handler that returns success without actually approving or declining the
SpeakerEditPermissionRequestwould still satisfy these tests. After the GET, reload the request and assert the request was consumed or transitioned to the expected state.Also applies to: 1865-1871
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitSpeakersApiTest.php` around lines 1833 - 1839, The test currently only asserts a 200 status after calling the public approve endpoint; update it to reload the SpeakerEditPermissionRequest associated with $other_speaker and $token (e.g. fetch by token or by speaker id and pending state) and assert that its state transitioned to "approved" (or was consumed/deleted) after the GET; modify the block around the GET call that uses "/api/public/v1/speakers/{$other_speaker->getId()}/edit-permission/{$token}/approve" to fetch the request model (e.g. SpeakerEditPermissionRequest::findByToken or repository method) and assert the expected state/consumed flag in addition to the 200, and apply the same change to the similar test block at 1865-1871.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bff604bc-ee84-4a22-96d3-120ac53684d3
📒 Files selected for processing (43)
.github/workflows/push.ymlapp/Http/Controllers/Apis/Protected/Main/OAuth2ElectionsApiController.phpapp/Models/Foundation/Summit/Registration/Attendees/SummitAttendee.phpapp/Services/Model/Imp/SummitTicketTypeService.phpdatabase/seeders/ApiEndpointsSeeder.phpdatabase/seeders/ConfigSeeder.phproutes/api_v1.phptests/BrowserKitTestCase.phptests/OAuth2AttendeesApiTest.phptests/OAuth2BookableRoomAttributeTypesApiTest.phptests/OAuth2ConsultantApiTest.phptests/OAuth2ElectionApiTest.phptests/OAuth2MembersApiTest.phptests/OAuth2OAuth2SponsorshipTypeApiTest.phptests/OAuth2OrganizationsApiTest.phptests/OAuth2PresentationApiTest.phptests/OAuth2PromoCodesApiTest.phptests/OAuth2SelectionPlansApiTest.phptests/OAuth2SummitBadgeTypeApiTest.phptests/OAuth2SummitBadgeViewTypeApiTest.phptests/OAuth2SummitBadgesApiTest.phptests/OAuth2SummitDocumentsApiControllerTest.phptests/OAuth2SummitEmailEventFlowApiControllerTest.phptests/OAuth2SummitEventsBulkActionsTest.phptests/OAuth2SummitLocationsApiTest.phptests/OAuth2SummitPromoCodesApiTest.phptests/OAuth2SummitSpeakersApiTest.phptests/OAuth2SummitSponsorApiTest.phptests/OAuth2SummitSponsorshipTypeApiControllerTest.phptests/OAuth2SummitSubmissionInvitationApiTest.phptests/OAuth2SummitSubmittersApiTest.phptests/OAuth2SummitTaxTypeApiTest.phptests/OAuth2SummitTicketTypesApiTest.phptests/OAuth2SummitTicketsApiTest.phptests/OAuth2SummitTrackChairsApiTest.phptests/OAuth2SummitTracksApiTest.phptests/OAuth2TrackGroupsApiTest.phptests/OAuth2TrackQuestionsTemplateTest.phptests/OAuth2TrackTagGroupsApiTest.phptests/OAuth2TracksApiTest.phptests/ProtectedApiTestCase.phptests/SummitRSVPInvitationServiceTest.phptests/SummitRSVPServiceTest.php
💤 Files with no reviewable changes (1)
- tests/OAuth2ConsultantApiTest.php
| - { name: "OAuth2AttendeesApiTest", filter: "--filter test/OAuth2AttendeesApiTest"} | ||
| - { name: "OAuth2SummitSponsorApiTest", filter: "--filter test/OAuth2SummitSponsorApiTest"} | ||
| - { name: "OAuth2BookableRoomAttributeTypesApiTest", filter: "--filter test/OAuth2BookableRoomAttributeTypesApiTest"} | ||
| - { name: "OAuth2CompaniesApiTest", filter: "--filter test/OAuth2CompaniesApiTest"} | ||
| - { name: "OAuth2ElectionsApiController", filter: "--filter test/OAuth2ElectionsApiController"} | ||
| - { name: "OAuth2MembersApiTest", filter: "--filter test/OAuth2MembersApiTest"} | ||
| - { name: "OAuth2GroupsApiTest", filter: "--filter test/OAuth2GroupsApiTest"} | ||
| - { name: "OAuth2OAuth2SponsorshipTypeApiTest", filter: "--filter test/OAuth2OAuth2SponsorshipTypeApiTest"} | ||
| - { name: "OAuth2OrganizationsApiTest", filter: "--filter test/OAuth2OrganizationsApiTest"} | ||
| - { name: "OAuth2PresentationApiTest", filter: "--filter test/OAuth2PresentationApiTest"} | ||
| - { name: "OAuth2PromoCodesApiTest", filter: "--filter test/OAuth2PromoCodesApiTest"} | ||
| - { name: "OAuth2SummitPromoCodesApiTest", filter: "--filter test/OAuth2SummitPromoCodesApiTest"} | ||
| - { name: "OAuth2SelectionPlansApiTest", filter: "--filter test/OAuth2SelectionPlansApiTest"} | ||
| - { name: "OAuth2SummitSpeakersApiTest", filter: "--filter test/OAuth2SummitSpeakersApiTest" } | ||
| - { name: "OAuth2SummitBadgesApiTest", filter: "--filter test/OAuth2SummitBadgesApiTest" } | ||
| - { name: "OAuth2TracksApiTest", filter: "--filter test/OAuth2TracksApiTest" } | ||
| - { name: "OAuth2TrackTagGroupsApiTest", filter: "--filter test/OAuth2TrackTagGroupsApiTest" } | ||
| - { name: "OAuth2SummitTracksApiTest", filter: "--filter test/OAuth2SummitTracksApiTest" } | ||
| - { name: "OAuth2TagsApiTest", filter: "--filter test/OAuth2TagsApiTest" } | ||
| - { name: "OAuth2SummitSponsorshipApiControllerTest", filter: "--filter test/OAuth2SummitSponsorshipApiControllerTest" } | ||
| - { name: "OAuth2SummitSponsorshipTypeApiControllerTest", filter: "--filter test/OAuth2SummitSponsorshipTypeApiControllerTest" } | ||
| - { name: "OAuth2SummitSubmissionInvitationApiTest", filter: "--filter test/OAuth2SummitSubmissionInvitationApiTest" } | ||
| - { name: "OAuth2SummitSubmittersApiTest", filter: "--filter test/OAuth2SummitSubmittersApiTest" } | ||
| - { name: "OAuth2SummitTaxTypeApiTest", filter: "--filter test/OAuth2SummitTaxTypeApiTest" } | ||
| - { name: "OAuth2SummitTicketTypesApiTest", filter: "--filter tests/OAuth2SummitTicketTypesApiTest" } | ||
| - { name: "OAuth2SummitTicketsApiTest", filter: "--filter tests/OAuth2SummitTicketsApiTest" } | ||
| - { name: "OAuth2SummitTrackChairsApiTest", filter: "--filter tests/OAuth2SummitTrackChairsApiTest" } | ||
| - { name: "OAuth2SummitTrackChairsRankingApiTest", filter: "--filter tests/OAuth2SummitTrackChairsRankingApiTest" } | ||
| - { name: "OAuth2TrackGroupsApiTest", filter: "--filter tests/OAuth2TrackGroupsApiTest" } | ||
| - { name: "OAuth2TrackQuestionsTemplateTest", filter: "--filter tests/OAuth2TrackQuestionsTemplateTest" } | ||
| - { name: "OAuth2SummitBadgeTypeApiTest", filter: "--filter tests/OAuth2SummitBadgeTypeApiTest" } | ||
| - { name: "OAuth2SummitBadgeViewTypeApiTest", filter: "--filter tests/OAuth2SummitBadgeViewTypeApiTest" } | ||
| - { name: "OAuth2SummitDocumentsApiControllerTest", filter: "--filter tests/OAuth2SummitDocumentsApiControllerTest" } | ||
| - { name: "OAuth2SummitEmailEventFlowApiControllerTest", filter: "--filter tests/OAuth2SummitEmailEventFlowApiControllerTest" } | ||
| - { name: "OAuth2SummitEventsBulkActionsTest", filter: "--filter tests/OAuth2SummitEventsBulkActionsTest" } | ||
| - { name: "OAuth2SummitLocationsApiTest", filter: "--filter tests/OAuth2SummitLocationsApiTest" } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Workflow rows using file-like values inside --filter:"
python - <<'PY'
from pathlib import Path
for i, line in enumerate(Path(".github/workflows/push.yml").read_text().splitlines(), 1):
if "--filter test/" in line or "--filter tests/" in line:
print(f"{i}: {line.strip()}")
PY
echo
echo "Matching test files present in the repo:"
fd -t f 'OAuth2*Test.php' tests | sortRepository: OpenStackweb/summit-api
Length of output: 3938
🌐 Web query:
How does PHPUnit's --filteroption work? Does it accept file paths liketests/OAuth2SummitLocationsApiTest, or does it only match test names / regex patterns?
💡 Result:
phpunit --filter <pattern> filters by test name, not by file path. It matches a regular-expression pattern against the test’s “name string” (typically Fully\Qualified\TestClass::testMethod, and for data providers it includes the data set suffix). If you don’t put regex delimiters around the pattern, PHPUnit wraps it in /.../. [1], [2]
So --filter tests/OAuth2SummitLocationsApiTest will only work if that literal text happens to appear in the test name (it usually won’t, because test names use namespaces \ and ::, not filesystem paths with /). [1], [2]
To target a file, pass the file path as an argument (separately from --filter), e.g. phpunit --filter OAuth2SummitLocationsApiTest tests/OAuth2SummitLocationsApiTest.php. [3]
Sources: [1] PHPUnit manual (TextUI, --filter) [2] PHPUnit TextUI docs (filter test name formats) [3] Example of --filter used alongside a test file argument
Fix PHPUnit filter syntax—these will not match any tests.
The --filter values at lines 72–107 use file-path syntax (e.g., --filter test/OAuth2AttendeesApiTest, --filter tests/OAuth2SummitLocationsApiTest), but PHPUnit's --filter option matches test names by regex (like Fully\Qualified\TestClass::testMethod), not file paths. These filters will match zero tests, causing all these matrix entries to pass silently without running anything.
To run tests from a specific file, use the file path as a separate argument instead: phpunit --filter OAuth2SummitLocationsApiTest tests/OAuth2SummitLocationsApiTest.php.
Additionally, no test files matching the pattern OAuth2*Test.php were found in the repository, suggesting these tests may not yet exist or are located elsewhere.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/push.yml around lines 72 - 107, The matrix entries use
PHPUnit's --filter with file-path-like values (e.g.,
"test/OAuth2AttendeesApiTest", "tests/OAuth2SummitLocationsApiTest") which won't
match test names; update each entry (references: OAuth2AttendeesApiTest,
OAuth2SummitLocationsApiTest, etc.) to either provide a proper regex test name
to --filter (like "Fully\\Qualified\\ClassName::testMethod") or remove the
--filter and pass the test file path as a separate phpunit argument (e.g.,
phpunit --filter OAuth2SummitLocationsApiTest
tests/OAuth2SummitLocationsApiTest.php), and also normalize "test" vs "tests"
directory names to the actual repo layout so the matrix runs the intended files.
| 'name' => 'get-attendees-votes', | ||
| 'route' => '/api/v1/summits/{id}/presentations/{id}/attendee-votes', | ||
| 'route' => '/api/v1/summits/{id}/presentations/{presentation_id}/attendee-votes', |
There was a problem hiding this comment.
Make these route fixes update existing seeded rows too.
These string fixes only take effect on a fresh database. Line 9359 always creates a new ApiEndpoint, and Lines 9386-9389 persist it without reconciling an existing row, so old installs keep the stale routes and can accumulate duplicates for the same logical endpoint. Please pair this with an upsert/data-migration path keyed by API + endpoint name, otherwise the hotfix will not actually repair previously seeded environments.
Also applies to: 6727-6728
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@database/seeders/ApiEndpointsSeeder.php` around lines 6510 - 6511, The seeder
currently always creates a new ApiEndpoint record with the corrected 'route' (so
old installs keep stale routes and duplicates); update the ApiEndpointsSeeder
logic to reconcile existing rows by keying on unique identifiers (e.g., 'api'
and 'name') and perform an upsert/update instead of blind create — use
Eloquent's updateOrCreate or firstOrNew + save to locate an existing ApiEndpoint
by ['api' => $api, 'name' => 'get-attendees-votes'] and then set/update the
'route' and other fields before saving; apply the same upsert change to the
other similar seeding blocks that seed endpoints (the other block that updates
get-attendees-votes and the block referenced around the earlier/other route
fixes) so the migration will modify existing seeded rows rather than inserting
duplicates.
| /** | ||
| * Creates an individual selection list and assigns a presentation to it via ORM. | ||
| * Bypasses API because createIndividualSelectionList service uses getById() | ||
| * with the token's user_id (external ID) instead of getByExternalId(). | ||
| */ | ||
| private function createSelectionListAndAssignPresentation(string $collection = SummitSelectedPresentation::CollectionSelected): SummitSelectedPresentationList | ||
| { | ||
| $selection_list = self::$default_selection_plan->createIndividualSelectionList( | ||
| self::$defaultTrack, self::$member | ||
| ); | ||
|
|
||
| $selection = SummitSelectedPresentation::create( | ||
| $selection_list, | ||
| self::$presentations[0], | ||
| $collection, | ||
| self::$member | ||
| ); | ||
| $selection->setOrder(1); | ||
| $selection_list->addSelection($selection); | ||
|
|
||
| self::$em->persist(self::$summit); | ||
| self::$em->flush(); | ||
|
|
||
| return $selection_list; | ||
| } |
There was a problem hiding this comment.
Don't bake the known identity-lookup bug into the fixture path.
The inline note says the API/service still resolves the authenticated user with getById() instead of getByExternalId(). Seeding through the ORM keeps these tests green, but it also means this suite never exercises that broken path. Please fix the lookup or add a dedicated regression test for the API behavior before merging.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SelectionPlansApiTest.php` around lines 30 - 54, The test is
bypassing a known identity-lookup bug by seeding via ORM in
createSelectionListAndAssignPresentation; update the code so the service/API
uses getByExternalId() instead of getById() when resolving the authenticated
user within createIndividualSelectionList (or the controller that authenticates
the token), ensuring the real API path is exercised; if you prefer not to change
service code now, add a dedicated regression test that exercises the API
endpoint for creating an individual selection list (call the API with an auth
token tied to an external ID and assert it resolves correctly), referencing
createIndividualSelectionList, getById, and getByExternalId to locate the
relevant code to change or the endpoint to test.
| $document = self::$summit->getSummitDocuments()[0]; | ||
| $params = [ | ||
| 'id' => self::$summit->getId(), | ||
| 'document_id' => $document->getId() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether the relevant ORM associations define explicit ordering.
# Expected result:
# - If no OrderBy is present on summit documents / event types associations,
# numeric offsets in the tests are not guaranteed to be stable.
fd 'Summit*.php$' app | xargs -r rg -n -C4 'getSummitDocuments|getEventTypes|OrderBy|@ORM\\OrderBy|OneToMany|ManyToMany'Repository: OpenStackweb/summit-api
Length of output: 29232
🏁 Script executed:
cat -n tests/OAuth2SummitDocumentsApiControllerTest.php | head -150Repository: OpenStackweb/summit-api
Length of output: 5707
🏁 Script executed:
sed -n '275,290p; 325,340p' tests/OAuth2SummitDocumentsApiControllerTest.phpRepository: OpenStackweb/summit-api
Length of output: 1156
Avoid positional access on ORM collections; add assertions for DELETE operations.
The tests access getSummitDocuments()[0/1] and getEventTypes()[0], which depends on collection ordering without explicit @ORM\OrderBy constraints in the model associations. This makes tests flaky across different ORM hydration cycles. Instead, store created fixtures in test properties or resolve by unique field (e.g., document name).
Additionally, the DELETE tests (lines 280–336) only assert HTTP status without verifying the relationship was actually removed, allowing regressions to slip through. Add post-condition assertions to confirm the unlink operation succeeded.
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 114-114: Avoid unused local variables such as '$summit'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SummitDocumentsApiControllerTest.php` around lines 114 - 117,
Replace fragile positional collection access with deterministic lookups: stop
using getSummitDocuments()[0], getSummitDocuments()[1], and getEventTypes()[0]
in tests and instead retrieve fixtures by a stored test property or by querying
the collection for a unique attribute (e.g., document name or event type name)
before calling controller methods (refer to getSummitDocuments() and
getEventTypes() usages). For the DELETE tests, after asserting the HTTP status,
add assertions that the relationship was actually removed by reloading the
summit entity or its collection and asserting the document/event type is no
longer present (verify by ID or unique field), ensuring the unlink operation
succeeded. Ensure any created fixtures are saved to test properties when created
so tests reference them deterministically rather than relying on collection
order.
| $content = $response->getContent(); | ||
| $this->assertResponseStatus(201); | ||
| $email_event = json_decode($content); | ||
| $this->assertNotNull($email_event); | ||
| } |
There was a problem hiding this comment.
Assert the update actually changed the resource.
assertNotNull($email_event) only proves the endpoint returned JSON. If the controller ignores recipient or email_template_identifier, this test still passes. Please assert the updated fields in the response, and ideally re-fetch the event to verify persistence.
Proposed assertion strengthening
$this->assertResponseStatus(201);
$email_event = json_decode($content);
$this->assertNotNull($email_event);
+ $this->assertEquals($data['email_template_identifier'], $email_event->email_template_identifier);
+ $this->assertEquals($data['recipient'], $email_event->recipient);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $content = $response->getContent(); | |
| $this->assertResponseStatus(201); | |
| $email_event = json_decode($content); | |
| $this->assertNotNull($email_event); | |
| } | |
| $content = $response->getContent(); | |
| $this->assertResponseStatus(201); | |
| $email_event = json_decode($content); | |
| $this->assertNotNull($email_event); | |
| $this->assertEquals($data['email_template_identifier'], $email_event->email_template_identifier); | |
| $this->assertEquals($data['recipient'], $email_event->recipient); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SummitEmailEventFlowApiControllerTest.php` around lines 120 -
124, The test in OAuth2SummitEmailEventFlowApiControllerTest currently only
checks json exists; update it to assert that the response contains the expected
updated fields (e.g., verify $email_event->recipient and
$email_event->email_template_identifier match the values sent in the request)
and then re-fetch the email event via the controller/API (using the same ID from
$email_event) to assert those fields were persisted; locate references to
$response and $email_event in the test and replace the loose
assertNotNull($email_event) with concrete field assertions and a follow-up GET
request to verify persistence.
| // Delete may return 204 (success) or 400 (seeded events cannot be deleted) | ||
| $this->assertTrue(in_array($response->getStatusCode(), [204, 400])); |
There was a problem hiding this comment.
Make the delete expectation deterministic.
Allowing both 204 and 400 makes this test too weak: a broken delete path can still pass as long as it returns 400. Since setUp() seeds the same fixture every time, this test should assert one expected outcome. Either validate that seeded events are intentionally protected with a specific 400 response, or create a deletable event and assert 204 plus a follow-up lookup failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SummitEmailEventFlowApiControllerTest.php` around lines 144 -
145, The test currently allows both 204 and 400 for the DELETE response which
makes it nondeterministic; update the test in
OAuth2SummitEmailEventFlowApiControllerTest so it asserts a single deterministic
outcome: either change the expectation to assertEquals(400,
$response->getStatusCode()) if seeded events are intentionally protected (and
add an explanatory comment), or (preferred) create a fresh deletable event in
setUp() or inside the test, call the delete on that new event and
assertEquals(204, $response->getStatusCode()), then perform a follow-up GET on
the deleted event and assert it returns 404 to ensure deletion actually occurred
(use the same $response variable and the test’s delete call to locate where to
change behavior).
|
|
||
| $this->assertResponseStatus(204); | ||
| $content = $response->getContent(); | ||
| $this->assertResponseStatus(201); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the updateEvents controller method to verify expected status code
ast-grep --pattern $'public function updateEvents($$$) {
$$$
}'Repository: OpenStackweb/summit-api
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Search for updateEvents method using rg
rg "updateEvents" --type php -B 2 -A 10Repository: OpenStackweb/summit-api
Length of output: 8598
🏁 Script executed:
#!/bin/bash
# Look at the test file to understand the context
cat -n tests/OAuth2SummitEventsBulkActionsTest.php | head -80Repository: OpenStackweb/summit-api
Length of output: 2889
🏁 Script executed:
#!/bin/bash
# Find the base controller class and the updated() method
rg "function updated" --type php -B 2 -A 5Repository: OpenStackweb/summit-api
Length of output: 1848
🏁 Script executed:
#!/bin/bash
# Look for the controller hierarchy - check extends clause
rg "class OAuth2SummitEventsApiController" --type php -A 5Repository: OpenStackweb/summit-api
Length of output: 683
Reconsider HTTP status code 201 for bulk update operation.
The updateEvents() method calls $this->updated() with default parameters, which returns HTTP 201 with a JSON response body containing only 'ok'. HTTP 201 (Created) is semantically incorrect for an update operation. Since the endpoint does not return updated resources, use HTTP 200 (OK) or HTTP 204 (No Content) instead. If returning updated resources in the response body is planned, then 200 OK would be appropriate; otherwise, use 204 No Content or configure the method with $this->updated(null, false) to return 204.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SummitEventsBulkActionsTest.php` at line 71, The test asserts 201
but updateEvents() calls $this->updated() which returns 201 by default and is
wrong for an update; either change the controller call to $this->updated(null,
false) so updateEvents() returns 204 No Content, or adjust the test assertion to
expect 200 OK if you plan to return the updated resources in the body—update the
assertion in OAuth2SummitEventsBulkActionsTest.php to match the chosen behavior
(use assertResponseStatus(204) after switching to $this->updated(null, false),
or assertResponseStatus(200) if you modify updateEvents() to return updated
resources).
| // Invalid class names return 412 validation error | ||
| $this->assertTrue(in_array($response->getStatusCode(), [200, 412])); | ||
|
|
||
| $banners = json_decode($content); | ||
| $this->assertTrue(!is_null($banners)); | ||
| $this->assertNotNull($banners); |
There was a problem hiding this comment.
Pick one contract for invalid banner filters.
Line 1082 says invalid class names should return 412, but Line 1083 also accepts 200. That makes this test non-signal: broken validation and silently ignored filters both pass.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SummitLocationsApiTest.php` around lines 1082 - 1086, The test
currently accepts both 200 and 412 for invalid banner class names which hides
failures; pick a single contract (use 412 for invalid filters) and update the
assertions accordingly: change the status check from allowing [200, 412] to
assertEquals(412, $response->getStatusCode()) (or an equivalent single-value
assertion), and remove or change the subsequent banner decoding/nonnull checks
($banners = json_decode($content); $this->assertNotNull($banners);) to instead
assert the expected error payload shape for validation failures (e.g., error
message/code fields) so the test only validates the 412 error path for invalid
banner filters.
| // Copy with empty data should succeed (copies all locations) | ||
| $this->assertTrue(in_array($response->getStatusCode(), [200, 201, 412])); |
There was a problem hiding this comment.
copy() currently passes on failure.
The comment says the empty payload should copy all locations, but the assertion also allows 412. If validation fails, this test still goes green and stops exercising the copy path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SummitLocationsApiTest.php` around lines 1321 - 1322, The test
currently allows a 412 status so copy() failures pass; update the assertion in
OAuth2SummitLocationsApiTest (the test that calls copy() and checks
$response->getStatusCode()) to disallow 412 by asserting the status is one of
the success codes only (e.g. assertTrue(in_array($response->getStatusCode(),
[200, 201])) or use assertContains/assertEquals for a specific expected code)
and optionally add an assertion on the response body to confirm locations were
copied.
| $other_speaker = new \models\summit\PresentationSpeaker(); | ||
| $other_speaker->setFirstName('EditPerm'); | ||
| $other_speaker->setLastName('Speaker'); | ||
| $other_speaker->setBio('Test bio'); | ||
| $other_speaker->setMember(self::$member2); | ||
| self::$em->persist($other_speaker); | ||
| self::$em->flush(); | ||
| self::$em->clear(); |
There was a problem hiding this comment.
Don't bind ad-hoc speakers to self::$member2.
PresentationSpeaker::setMember() also updates the inverse side ($member->setSpeaker($this)), so these setups are rewriting a shared fixture member, not just giving the synthetic speaker an email. That makes the suite order-dependent. Since PresentationSpeaker::getEmail() already falls back to registration_request, use a fresh member or a dedicated registration request instead of mutating self::$member2.
Also applies to: 1816-1822, 1848-1854
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SummitSpeakersApiTest.php` around lines 1192 - 1199, The test is
mutating the shared fixture by calling
PresentationSpeaker::setMember(self::$member2); instead create and attach either
a fresh Member instance or a dedicated RegistrationRequest for the synthetic
speaker so you don't overwrite the inverse relation on the shared member; update
the three occurrences (around lines shown) to stop using self::$member2 and
instead call $speaker->setMember($newMember) where $newMember is newly
instantiated and persisted, or set $speaker->setRegistrationRequest($newRequest)
and persist that, relying on PresentationSpeaker::getEmail() fallback to
registration_request.
*OAuth2RSVPSummitEventApiTest: rewrite to use InsertSummitTestData trait, remove tests for undefined controller actions (addEventRSVP on SummitMembersApiController), skip testDeleteEventRSVP due to unimplemented SummitService::unRSVPEvent() * OAuth2SpeakersAssistancesApiTest: rewrite to use InsertSummitTestData trait, use summit-bound speaker from presentations instead of self::$speaker which has no summit association, remove tests for commented-out routes (sendAnnouncementEmail) * OAuth2SponsoredProjectsApiTest: add InsertSummitTestData trait with local assets disk config to avoid Swift/OpenStack dependency, replace all hardcoded entity IDs with dynamically created test data, fix addSupportingCompany to use POST (not PUT) with company from test data, fix testUpdateSponsorshipType expected status (201 not 204) * OAuth2SummitAccessLevelTypeTest: rewrite to use InsertSummitTestData trait instead of hardcoded summit ID 27 * OAuth2SummitAdministratorPermissionGroupApiControllerTest: add missing Mockery import, fix route param name (group_id not id) in testGetByIdOK, add parent::tearDown() call
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
.github/workflows/push.yml (1)
72-117:⚠️ Potential issue | 🟠 MajorUse class-name filters here, not
test/ortests/paths.These new matrix entries repeat the same PHPUnit
--filterproblem already raised earlier: values like--filter test/OAuth2AttendeesApiTestand--filter tests/OAuth2SummitLocationsApiTestwill usually match no tests, so those jobs can pass without executing the intended suite. Keep the filter to the test class name only, consistent with the existing entries above.Suggested fix
- - { name: "OAuth2AttendeesApiTest", filter: "--filter test/OAuth2AttendeesApiTest"} + - { name: "OAuth2AttendeesApiTest", filter: "--filter OAuth2AttendeesApiTest" } - - { name: "OAuth2SummitSponsorApiTest", filter: "--filter test/OAuth2SummitSponsorApiTest"} + - { name: "OAuth2SummitSponsorApiTest", filter: "--filter OAuth2SummitSponsorApiTest" } - - { name: "OAuth2SummitTicketTypesApiTest", filter: "--filter tests/OAuth2SummitTicketTypesApiTest" } + - { name: "OAuth2SummitTicketTypesApiTest", filter: "--filter OAuth2SummitTicketTypesApiTest" } - - { name: "OAuth2SummitLocationsApiTest", filter: "--filter tests/OAuth2SummitLocationsApiTest" } + - { name: "OAuth2SummitLocationsApiTest", filter: "--filter OAuth2SummitLocationsApiTest" }Apply the same change across the rest of the new entries.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/push.yml around lines 72 - 117, The matrix entries use PHPUnit filters like "--filter test/OAuth2AttendeesApiTest" which include path prefixes and will usually match nothing; update each new job entry (e.g., the entries named "OAuth2AttendeesApiTest", "OAuth2SummitTicketsApiTest", "OAuth2SummitLocationsApiTest", etc.) to use the test class name only in the filter (e.g., "--filter OAuth2AttendeesApiTest") so the PHPUnit runner matches the intended test class; apply this same change consistently across all the new matrix items shown.
🧹 Nitpick comments (3)
tests/OAuth2RSVPSummitEventApiTest.php (1)
38-41: Test class has no active tests.With
testAddRSVP()removed andtestDeleteEventRSVP()skipped, this class provides no test coverage. The infrastructure (trait, setUp, tearDown) is in place but unused.Consider tracking
SummitService::unRSVPEvent()implementation as a backlog item to ensure this test gets enabled eventually.Would you like me to open an issue to track the implementation of
SummitService::unRSVPEvent()and re-enabling this test?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2RSVPSummitEventApiTest.php` around lines 38 - 41, The test class currently has no active tests because testAddRSVP() was removed and testDeleteEventRSVP() is skipped; to fix, either re-introduce a functional RSVP test (restore testAddRSVP() or add a new test that exercises RSVP flow) or implement SummitService::unRSVPEvent() and re-enable testDeleteEventRSVP() by removing the markTestSkipped call; locate testDeleteEventRSVP(), any existing testAddRSVP() remnants, the test class setUp/tearDown and the trait used by the class to wire dependencies and ensure the test invokes SummitService::unRSVPEvent() through the same service instance so the test becomes active again (or if you intentionally want no tests, remove the empty test class).tests/OAuth2SpeakersAssistancesApiTest.php (1)
133-133: Consider usingassertNotEmptyfor clarity.Using
assertNotEmpty($content)is more idiomatic and provides a clearer failure message thanassertTrue(!empty($content)).♻️ Suggested refactor
- $this->assertTrue(!empty($content)); + $this->assertNotEmpty($content);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SpeakersAssistancesApiTest.php` at line 133, Replace the negated empty assertion with the PHPUnit helper: in the OAuth2SpeakersAssistancesApiTest test where the variable $content is asserted, change the line using $this->assertTrue(!empty($content)) to use $this->assertNotEmpty($content) so the intent is clearer and PHPUnit will produce a better failure message.tests/OAuth2SponsoredProjectsApiTest.php (1)
222-274: Consider extracting sponsorship type creation to a helper method.The sponsorship type creation logic (lines 227-251) is duplicated across
testUpdateSponsorshipType,testGetAllSponsorshipTypes,testAddSupportingCompany, andtestGetAllSupportingCompanies. Extracting this to a helper liketestAddSponsorshipType()(similar totestAddSponsoredProject()) would reduce duplication and improve maintainability.♻️ Proposed helper method
private function createSponsorshipType($projectId) { $data = [ 'name' => str_random(16).' sponsorship type', 'description' => str_random(16).' sponsorship type description', 'is_active' => true, ]; $headers = [ "HTTP_Authorization" => " Bearer " . $this->access_token, "CONTENT_TYPE" => "application/json" ]; $response = $this->action( "POST", "OAuth2SponsoredProjectApiController@addSponsorshipType", ['id' => $projectId], [], [], [], $headers, json_encode($data) ); $this->assertResponseStatus(201); return json_decode($response->getContent()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SponsoredProjectsApiTest.php` around lines 222 - 274, Extract the repeated sponsorship type creation block into a private helper method (e.g., createSponsorshipType) that accepts the project id, builds the $data and $headers, performs the POST via OAuth2SponsoredProjectApiController@addSponsorshipType, asserts the 201 response and returns json_decoded content; then replace the duplicated code in testUpdateSponsorshipType and the other tests (testGetAllSponsorshipTypes, testAddSupportingCompany, testGetAllSupportingCompanies) to call createSponsorshipType($project->id) and use its returned sponsorship type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/OAuth2SpeakersAssistancesApiTest.php`:
- Line 198: The test asserts a 201 for PUT updates but the helper updated() in
JsonController should return 200 for updates with content; update
JsonController::updated($data = 'ok', $has_content = true) to use HTTP 200 when
$has_content is true (and 204 when false), then change the assertion in
OAuth2SpeakersAssistancesApiTest:: where $this->assertResponseStatus(201) is
used for PUT update to expect 200 (and update any OpenAPI docs/examples to
reflect the new 200 response for PUT updates).
In `@tests/OAuth2SponsoredProjectsApiTest.php`:
- Around line 589-596: The test assumes $subproject->getParentProject() returns
a SponsoredProject but it is nullable; add a defensive null check/assertion
after retrieving $parent_project from getParentProject() (e.g., assertNotNull or
guard) before calling $parent_project->getId() so the test fails with a clear
message if no parent exists; update the block around getParentProject(),
$parent_project and the later use of getId() to ensure $parent_project is
non-null.
In `@tests/OAuth2SummitAdministratorPermissionGroupApiControllerTest.php`:
- Around line 14-15: Remove the unused Mockery import and explicit closure:
delete the top-level "use Mockery;" import and remove the "Mockery::close()"
call in the tearDown method of the
OAuth2SummitAdministratorPermissionGroupApiControllerTest class (the test
methods like testAddGroupOK, testUpdateGroupOk, etc. do not create any Mockery
mocks and the parent ProtectedApiTestCase handles Mockery lifecycle). Also scan
the class for any other direct Mockery usage and remove it if unused.
---
Duplicate comments:
In @.github/workflows/push.yml:
- Around line 72-117: The matrix entries use PHPUnit filters like "--filter
test/OAuth2AttendeesApiTest" which include path prefixes and will usually match
nothing; update each new job entry (e.g., the entries named
"OAuth2AttendeesApiTest", "OAuth2SummitTicketsApiTest",
"OAuth2SummitLocationsApiTest", etc.) to use the test class name only in the
filter (e.g., "--filter OAuth2AttendeesApiTest") so the PHPUnit runner matches
the intended test class; apply this same change consistently across all the new
matrix items shown.
---
Nitpick comments:
In `@tests/OAuth2RSVPSummitEventApiTest.php`:
- Around line 38-41: The test class currently has no active tests because
testAddRSVP() was removed and testDeleteEventRSVP() is skipped; to fix, either
re-introduce a functional RSVP test (restore testAddRSVP() or add a new test
that exercises RSVP flow) or implement SummitService::unRSVPEvent() and
re-enable testDeleteEventRSVP() by removing the markTestSkipped call; locate
testDeleteEventRSVP(), any existing testAddRSVP() remnants, the test class
setUp/tearDown and the trait used by the class to wire dependencies and ensure
the test invokes SummitService::unRSVPEvent() through the same service instance
so the test becomes active again (or if you intentionally want no tests, remove
the empty test class).
In `@tests/OAuth2SpeakersAssistancesApiTest.php`:
- Line 133: Replace the negated empty assertion with the PHPUnit helper: in the
OAuth2SpeakersAssistancesApiTest test where the variable $content is asserted,
change the line using $this->assertTrue(!empty($content)) to use
$this->assertNotEmpty($content) so the intent is clearer and PHPUnit will
produce a better failure message.
In `@tests/OAuth2SponsoredProjectsApiTest.php`:
- Around line 222-274: Extract the repeated sponsorship type creation block into
a private helper method (e.g., createSponsorshipType) that accepts the project
id, builds the $data and $headers, performs the POST via
OAuth2SponsoredProjectApiController@addSponsorshipType, asserts the 201 response
and returns json_decoded content; then replace the duplicated code in
testUpdateSponsorshipType and the other tests (testGetAllSponsorshipTypes,
testAddSupportingCompany, testGetAllSupportingCompanies) to call
createSponsorshipType($project->id) and use its returned sponsorship type.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 05af53c7-1871-4add-8baf-6605ec4b2fdb
📒 Files selected for processing (6)
.github/workflows/push.ymltests/OAuth2RSVPSummitEventApiTest.phptests/OAuth2SpeakersAssistancesApiTest.phptests/OAuth2SponsoredProjectsApiTest.phptests/OAuth2SummitAccessLevelTypeTest.phptests/OAuth2SummitAdministratorPermissionGroupApiControllerTest.php
| @@ -273,16 +196,17 @@ public function testUpdateSummitAssistance($summit_id = 23){ | |||
|
|
|||
| $content = $response->getContent(); | |||
| $this->assertResponseStatus(201); | |||
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check what status code the updateSpeakerSummitAssistance action returns
# Find the controller method implementation
ast-grep --pattern $'function updateSpeakerSummitAssistance($$$) {
$$$
}'
# Also search for response status in the controller
rg -n "updateSpeakerSummitAssistance" --type php -A 30 | head -100Repository: OpenStackweb/summit-api
Length of output: 9836
🏁 Script executed:
cat -n app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitSpeakersAssistanceApiController.php | sed -n '474,530p'Repository: OpenStackweb/summit-api
Length of output: 3037
🏁 Script executed:
rg -n "function updated" --type php -A 5 app/Http/Controllers/Repository: OpenStackweb/summit-api
Length of output: 575
Fix HTTP status code for PUT update operation.
The API intentionally returns 201 Created for PUT updates via the updated() helper method, but this violates HTTP semantics. PUT updates should return 200 OK (with response body) or 204 No Content, not 201 Created. Consider refactoring the updated() helper to return 200 for updates with content:
// app/Http/Controllers/JsonController.php
protected function updated($data = 'ok', $has_content = true)
{
$res = Response::json($data, $has_content ? 200 : 204); // Change 201 to 200
// ...
}Then update the OpenAPI documentation and test accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SpeakersAssistancesApiTest.php` at line 198, The test asserts a
201 for PUT updates but the helper updated() in JsonController should return 200
for updates with content; update JsonController::updated($data = 'ok',
$has_content = true) to use HTTP 200 when $has_content is true (and 204 when
false), then change the assertion in OAuth2SpeakersAssistancesApiTest:: where
$this->assertResponseStatus(201) is used for PUT update to expect 200 (and
update any OpenAPI docs/examples to reflect the new 200 response for PUT
updates).
| $parent_project = $subproject->getParentProject(); | ||
|
|
||
| $params = [ | ||
| 'id' => $subproject->getId(), | ||
| ]; | ||
|
|
||
| $data = [ | ||
| 'parent_project_id' => 1, | ||
| 'parent_project_id' => $parent_project->getId(), |
There was a problem hiding this comment.
Add null check for nullable getParentProject() return.
getParentProject() returns ?SponsoredProject (nullable), so calling $parent_project->getId() on line 596 could cause a null pointer exception if the parent is null. While the test creates the subproject with a parent, adding a defensive assertion improves test clarity and failure messages.
🛡️ Proposed fix
$parent_project = $subproject->getParentProject();
+ $this->assertNotNull($parent_project, 'Subproject should have a parent project');
$params = [
'id' => $subproject->getId(),
];
$data = [
'parent_project_id' => $parent_project->getId(),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $parent_project = $subproject->getParentProject(); | |
| $params = [ | |
| 'id' => $subproject->getId(), | |
| ]; | |
| $data = [ | |
| 'parent_project_id' => 1, | |
| 'parent_project_id' => $parent_project->getId(), | |
| $parent_project = $subproject->getParentProject(); | |
| $this->assertNotNull($parent_project, 'Subproject should have a parent project'); | |
| $params = [ | |
| 'id' => $subproject->getId(), | |
| ]; | |
| $data = [ | |
| 'parent_project_id' => $parent_project->getId(), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SponsoredProjectsApiTest.php` around lines 589 - 596, The test
assumes $subproject->getParentProject() returns a SponsoredProject but it is
nullable; add a defensive null check/assertion after retrieving $parent_project
from getParentProject() (e.g., assertNotNull or guard) before calling
$parent_project->getId() so the test fails with a clear message if no parent
exists; update the block around getParentProject(), $parent_project and the
later use of getId() to ensure $parent_project is non-null.
| use Mockery; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n tests/OAuth2SummitAdministratorPermissionGroupApiControllerTest.php | head -50Repository: OpenStackweb/summit-api
Length of output: 1898
🏁 Script executed:
# Check if Laravel testing base class handles Mockery
rg -A 10 "tearDown" --type php tests/ | head -80Repository: OpenStackweb/summit-api
Length of output: 5171
🏁 Script executed:
# Find and examine the referenced comparison file
cat -n tests/OAuth2SummitTrackChairsApiTest.php | sed -n '30,45p'Repository: OpenStackweb/summit-api
Length of output: 672
🏁 Script executed:
# Check Laravel version
cat composer.json | grep -A 5 -B 5 '"laravel/framework"'Repository: OpenStackweb/summit-api
Length of output: 525
🏁 Script executed:
# Find and examine the ProtectedApiTestCase
fd -t f "ProtectedApiTestCase" tests/Repository: OpenStackweb/summit-api
Length of output: 97
🏁 Script executed:
# Get more context on the base test case
rg -l "class ProtectedApiTestCase" --type php tests/Repository: OpenStackweb/summit-api
Length of output: 97
🏁 Script executed:
# Search for where ProtectedApiTestCase is defined
rg "class ProtectedApiTestCase" --type php -A 10Repository: OpenStackweb/summit-api
Length of output: 650
🏁 Script executed:
# Check BrowserKitTestCase
cat -n tests/BrowserKitTestCase.php | head -60Repository: OpenStackweb/summit-api
Length of output: 2413
🏁 Script executed:
# Look for tearDown in BrowserKitTestCase
rg "tearDown" tests/BrowserKitTestCase.php -A 5Repository: OpenStackweb/summit-api
Length of output: 49
🏁 Script executed:
# Check what the actual Laravel 12 testing base implements
# First, look for any custom configuration that might affect Mockery handling
rg "Mockery" tests/ --type php | head -30Repository: OpenStackweb/summit-api
Length of output: 3043
🏁 Script executed:
# Check the full BrowserKitTestCase file
wc -l tests/BrowserKitTestCase.phpRepository: OpenStackweb/summit-api
Length of output: 98
🏁 Script executed:
# Get the complete file
cat -n tests/BrowserKitTestCase.phpRepository: OpenStackweb/summit-api
Length of output: 3303
🏁 Script executed:
# Check if Laravel's BrowserKitTestCase has tearDown defined
# Look at what the imported BaseTestCase is
rg "class.*TestCase.*extends" tests/BrowserKitTestCase.phpRepository: OpenStackweb/summit-api
Length of output: 123
🏁 Script executed:
# Check what Laravel\BrowserKitTesting\TestCase might do with Mockery
# First, check if there's composer.lock or vendor information about laravel-browserkit-testing
rg "browserkit" composer.json -iRepository: OpenStackweb/summit-api
Length of output: 49
🏁 Script executed:
# Let's check if there are any phpunit or phpunit config files
fd -t f "phpunit\|\.xml" . | grep -E "(phpunit|\.xml)" | head -20Repository: OpenStackweb/summit-api
Length of output: 49
🏁 Script executed:
# Check if there's a phpunit.xml that might show test listeners or configuration for Mockery
cat phpunit.xml 2>/dev/null | head -50Repository: OpenStackweb/summit-api
Length of output: 923
🏁 Script executed:
# Look for any test setup or bootstrap files
fd -t f "TestCase\|bootstrap" tests/ | head -20Repository: OpenStackweb/summit-api
Length of output: 49
🏁 Script executed:
# Check what version of laravel-browserkit-testing is installed
rg "laravel.*browserkit" composer.json -i -A 2Repository: OpenStackweb/summit-api
Length of output: 49
🏁 Script executed:
# Check if laravel-browserkit-testing is in composer.lock
rg "laravel-browserkit-testing" composer.lock | head -5Repository: OpenStackweb/summit-api
Length of output: 49
🏁 Script executed:
# Let's understand what the actual parent tearDown does
# Check if there are any documented behaviors or actual tests that work without explicit Mockery::close()
rg "tearDown" tests/ -A 3 | grep -E "(tearDown|parent::tearDown|Mockery)" | head -50Repository: OpenStackweb/summit-api
Length of output: 3643
🏁 Script executed:
# Check Mockery documentation/behavior - does calling Mockery::close() twice cause issues?
# Let's examine how other tests handle this pattern
rg -B 3 -A 3 "parent::tearDown" tests/ | head -80Repository: OpenStackweb/summit-api
Length of output: 4444
🏁 Script executed:
# Check what version of laravel-browserkit-testing this uses
cat composer.json | grep -A 5 -B 5 "browserkit" -iRepository: OpenStackweb/summit-api
Length of output: 49
🏁 Script executed:
# Let's search for any documentation or comments about Mockery::close() in the test suite
rg "Mockery::close" tests/ -B 2 -A 2 | head -80Repository: OpenStackweb/summit-api
Length of output: 5141
🏁 Script executed:
# Check if the legacy BrowserKitTestCase might be causing the issue
# Look for any actual test failures or patterns in how it's used
rg "use Mockery" tests/ | wc -lRepository: OpenStackweb/summit-api
Length of output: 69
🏁 Script executed:
# Count how many tests explicitly use Mockery::close() vs those that don't
echo "Tests with Mockery::close():" && rg "Mockery::close" tests/ | wc -l && echo "Tests with 'use Mockery;':" && rg "use Mockery" tests/ | wc -lRepository: OpenStackweb/summit-api
Length of output: 128
🏁 Script executed:
# Let's check what the Laravel BrowserKitTesting package actually does
# First, see if we can understand the inheritance chain better
cat composer.lock | grep -A 20 '"name": "laravel/browser-kit-testing"'Repository: OpenStackweb/summit-api
Length of output: 1022
🏁 Script executed:
# Let's check if there are any test that ONLY have parent::tearDown() without explicit Mockery::close()
# and see if they work fine
rg "tearDown.*void" tests/ -A 3 | grep -E "tearDown|parent::tearDown|Mockery" | grep -v "^--$" > /tmp/teardown_patterns.txt && cat /tmp/teardown_patterns.txt | head -100Repository: OpenStackweb/summit-api
Length of output: 8428
🏁 Script executed:
# Count tests that have ONLY parent::tearDown() without explicit Mockery::close()
echo "=== Pattern: Only parent::tearDown() ===" && rg "tearDown.*void" tests/ -A 2 | grep -B 1 "parent::tearDown" | grep -v "Mockery" | grep "tearDown" | wc -lRepository: OpenStackweb/summit-api
Length of output: 109
🏁 Script executed:
# Let's check Laravel\BrowserKitTesting\TestCase source to understand what it does
# First, see if we can find it in vendor or get more info about how it handles tearDown
rg "tearDown" --type php | grep -i "browserkit\|testcase" | head -20Repository: OpenStackweb/summit-api
Length of output: 355
🏁 Script executed:
# Check if there's any actual evidence that Mockery::close() is needed
# by looking at tests that DON'T use it and see if they use Mockery mocks
rg -l "parent::tearDown" tests/ | while read file; do
if ! grep -q "Mockery::close" "$file"; then
if grep -q "Mockery::mock" "$file"; then
echo "Uses Mockery::mock but no explicit Mockery::close(): $file"
fi
fi
done | head -20Repository: OpenStackweb/summit-api
Length of output: 620
🏁 Script executed:
# Let's specifically check the test file to see if it actually uses Mockery mocks
grep -n "Mockery::mock\|Mockery::" tests/OAuth2SummitAdministratorPermissionGroupApiControllerTest.phpRepository: OpenStackweb/summit-api
Length of output: 95
🏁 Script executed:
# Check if the test file itself uses Mockery in the test methods
cat -n tests/OAuth2SummitAdministratorPermissionGroupApiControllerTest.php | grep -E "Mockery|testAdd|testGet|testUpdate|testDelete" | head -40Repository: OpenStackweb/summit-api
Length of output: 446
🏁 Script executed:
# Check what ProtectedApiTestCase does (since the file extends it indirectly)
cat -n tests/ProtectedApiTestCase.php | grep -A 5 "Mockery::mock"Repository: OpenStackweb/summit-api
Length of output: 296
🏁 Script executed:
# Verify: check if Laravel BrowserKitTestCase or ProtectedApiTestCase has a tearDown that handles Mockery
rg "class.*TestCase" tests/ProtectedApiTestCase.php -A 50 | grep -E "(tearDown|Mockery)" | head -10Repository: OpenStackweb/summit-api
Length of output: 131
🏁 Script executed:
# Let's check if Laravel's test base actually handles Mockery automatically
# by checking if there's a test listener or if parent::tearDown() in Laravel framework handles it
# First, let's see what the Laravel framework version has in terms of Mockery handling
web_search "Laravel 12 testing tearDown Mockery close automatic"Repository: OpenStackweb/summit-api
Length of output: 117
🏁 Script executed:
# Also check what the actual PHP unit configuration expects
grep -r "listener\|bootstrap" phpunit.xml | head -10Repository: OpenStackweb/summit-api
Length of output: 386
🏁 Script executed:
# Let's verify by checking a test that uses Mockery without explicit close to see if it works
cat -n tests/ProtectedApiTestCase.php | head -50Repository: OpenStackweb/summit-api
Length of output: 1998
🏁 Script executed:
# Let's check if the test file actually uses Mockery anywhere in the test methods
cat tests/OAuth2SummitAdministratorPermissionGroupApiControllerTest.php | grep -E "testAdd|testGet|testUpdate|testDelete" -A 30 | grep -i mockeryRepository: OpenStackweb/summit-api
Length of output: 49
🏁 Script executed:
# Check if any of the test methods actually use Mockery
grep -n "function test" tests/OAuth2SummitAdministratorPermissionGroupApiControllerTest.phpRepository: OpenStackweb/summit-api
Length of output: 232
🏁 Script executed:
# Let's look at a specific test method to see if it uses Mockery
sed -n '34,69p' tests/OAuth2SummitAdministratorPermissionGroupApiControllerTest.phpRepository: OpenStackweb/summit-api
Length of output: 1057
Remove the unused Mockery import and Mockery::close() call—this test file does not use Mockery mocks.
The test methods in this file (testAddGroupOK, testUpdateGroupOk, etc.) do not create any Mockery mocks, so neither the use Mockery; import nor the explicit Mockery::close() call is needed. The parent class ProtectedApiTestCase handles its own Mockery setup and does not require explicit closure in this child class's tearDown.
Suggested fix
<?php namespace Tests;
-use Mockery;
class OAuth2SummitAdministratorPermissionGroupApiControllerTest
extends ProtectedApiTestCase
{
public function tearDown():void
{
self::clearSummitTestData();
- Mockery::close();
parent::tearDown();
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SummitAdministratorPermissionGroupApiControllerTest.php` around
lines 14 - 15, Remove the unused Mockery import and explicit closure: delete the
top-level "use Mockery;" import and remove the "Mockery::close()" call in the
tearDown method of the OAuth2SummitAdministratorPermissionGroupApiControllerTest
class (the test methods like testAddGroupOK, testUpdateGroupOk, etc. do not
create any Mockery mocks and the parent ProtectedApiTestCase handles Mockery
lifecycle). Also scan the class for any other direct Mockery usage and remove it
if unused.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
tests/OAuth2SummitPresentationActionTypeApiTest.php (2)
214-245:⚠️ Potential issue | 🟡 MinorCapture the expected label before the request.
Line 245 compares the response to
self::$action2->getLabel(), which can be mutated by the same EntityManager during thePUT. That makes the assertion depend on Doctrine state instead of proving the payload was applied.Suggested fix
public function testUpdateAction(){ + $expectedLabel = self::$action2->getLabel()." UPDATE"; $params = [ 'id' => self::$summit->getId(), 'action_id' => self::$action2->getId(), ]; @@ $payload = [ - 'label' => self::$action2->getLabel()." UPDATE", + 'label' => $expectedLabel, ]; @@ $this->assertResponseStatus(201); $action = json_decode($content); $this->assertTrue(!is_null($action)); $this->assertTrue($action->id == self::$action2->getId()); - $this->assertTrue($action->label == self::$action2->getLabel()); + $this->assertSame($expectedLabel, $action->label); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitPresentationActionTypeApiTest.php` around lines 214 - 245, In testUpdateAction capture the expected label into a local variable before making the PUT request (e.g. $expectedLabel = self::$action2->getLabel()." UPDATE") so the test asserts against the payload value rather than the possibly mutating EntityManager state; then change the final assertion that currently compares $action->label to self::$action2->getLabel() to assert $action->label equals that $expectedLabel variable while keeping the other assertions (id, non-null) intact.
148-179:⚠️ Potential issue | 🟡 MinorAssert the reorder result, not only the id.
This test currently passes as long as the same entity comes back with a
201. It does not verify that theorderchange was applied.Suggested assertion
$this->assertResponseStatus(201); $action = json_decode($content); $this->assertTrue(!is_null($action)); - $this->assertTrue($action->id == self::$action2->getId()); + $this->assertSame(self::$action2->getId(), $action->id); + $this->assertSame(1, $action->order);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitPresentationActionTypeApiTest.php` around lines 148 - 179, The testReorderAction currently only checks the returned entity id; update it to assert the reorder actually occurred by verifying the returned object's order property (assert $action->order == 1) after the PUT to OAuth2SummitPresentationActionTypeApiController@update, and optionally load/inspect the other affected action (self::$action1/self::$action2) or the summit's action list to confirm relative ordering changed as expected; ensure the assertion uses the decoded $action from the response and keeps the existing status check.tests/OAuth2SummitOrdersApiTest.php (4)
1113-1121:⚠️ Potential issue | 🟡 MinorMissing null checks before accessing order and ticket.
Same issue as the previous delegation tests. Add null checks for consistency with the rest of the refactored test suite.
🛡️ Suggested fix
public function testDelegateAndUpdateEmail():void{ - $order = self::$summit->getOrders()[0]; + $order = self::$summit->getOrders()->first(); + if(is_null($order)) $this->markTestSkipped('No orders in test data'); $ticket = $order->getFirstTicket(); + if(is_null($ticket)) $this->markTestSkipped('No tickets in test data'); $params = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitOrdersApiTest.php` around lines 1113 - 1121, In testDelegateAndUpdateEmail add defensive null checks before using self::$summit->getOrders()[0] and $order->getFirstTicket(): verify self::$summit is set and has at least one order, assign $order only after that check, then verify $order is not null and has a ticket before assigning $ticket; if any check fails, call the same test-skip or fail helper used elsewhere in the suite (consistent with other delegation tests) so the test exits cleanly rather than dereferencing null values.
1001-1010:⚠️ Potential issue | 🟡 MinorMissing null checks before accessing order and ticket.
Unlike other refactored tests in this file (e.g.,
testUpdateTicket,testRevokeAttendee), this test accessesself::$summit->getOrders()[0]and$order->getFirstTicket()without null/empty checks. If the test data has no orders or tickets, this will produce an unclear error instead of a clean skip.🛡️ Suggested fix for consistency
public function testDoubleDelegate(){ - $order = self::$summit->getOrders()[0]; + $order = self::$summit->getOrders()->first(); + if(is_null($order)) $this->markTestSkipped('No orders in test data'); $ticket = $order->getFirstTicket(); + if(is_null($ticket)) $this->markTestSkipped('No tickets in test data'); $params = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitOrdersApiTest.php` around lines 1001 - 1010, In testDoubleDelegate update the setup to safely handle missing data by checking that self::$summit->getOrders() is non-empty and that $order->getFirstTicket() is not null before proceeding; if no orders or no ticket exist, call $this->markTestSkipped with a clear message. Locate the testDoubleDelegate method and add guards around the retrieval of $order (from self::$summit->getOrders()[0]) and $ticket (from $order->getFirstTicket()) to avoid dereferencing null/empty values and ensure consistent behavior with other tests like testUpdateTicket and testRevokeAttendee.
1051-1060:⚠️ Potential issue | 🟡 MinorMissing null checks before accessing order and ticket.
Same issue as
testDoubleDelegate—this test directly accessesself::$summit->getOrders()[0]and$order->getFirstTicket()without defensive checks. Apply the same null-check pattern used in other tests for consistency.🛡️ Suggested fix
public function testDelegateTicket(){ - $order = self::$summit->getOrders()[0]; + $order = self::$summit->getOrders()->first(); + if(is_null($order)) $this->markTestSkipped('No orders in test data'); $ticket = $order->getFirstTicket(); + if(is_null($ticket)) $this->markTestSkipped('No tickets in test data'); $params = [🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitOrdersApiTest.php` around lines 1051 - 1060, testDelegateTicket accesses self::$summit->getOrders()[0] and $order->getFirstTicket() without null/empty checks; add the same defensive checks used in testDoubleDelegate: verify self::$summit is set and has orders (check getOrders() is non-empty) before using [0], then verify $order is not null and that getFirstTicket() returned a ticket before proceeding (otherwise call $this->markTestSkipped or assertNotNull and return) so the test won't throw on missing data.
302-345:⚠️ Potential issue | 🔴 CriticalHardcoded
question_idvalues (3, 4, 5) are not created in test data.The validation rule for
extra_questionsonly checks structure (thatquestion_idis an integer andansweris a string), but does not validate that these questions actually exist. TheInsertSummitTestDatatrait creates only oneSummitSelectionPlanExtraQuestionTypewithout explicit ID assignment. Question IDs 3, 4, and 5 are never created, so the test will fail or produce unexpected behavior when updating orders with these hardcoded question references.Either create the required questions dynamically in test data and reference them by their actual IDs, or remove
extra_questionsfrom the payload if not essential to testing the order update functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitOrdersApiTest.php` around lines 302 - 345, The testUpdateOrder method is using hardcoded extra_questions question_id values (3,4,5) that aren't created by the test data; update the test to either create the missing SummitSelectionPlanExtraQuestionType entries via the InsertSummitTestData setup and use their actual IDs, or remove extra_questions from the payload if they are not required for this update scenario; specifically modify testUpdateOrder to obtain real question IDs (e.g., by creating questions in the InsertSummitTestData trait or querying self::$summit for its selection plan questions) and replace the hardcoded IDs in the extra_questions array, or drop the extra_questions key entirely.
🧹 Nitpick comments (3)
tests/OAuth2SummitMediaUploadTypeApiControllerTest.php (1)
57-69: Extract a shared media-upload-type payload builder.These four payloads now need the same
is_editablecontract field, and they already vary slightly in optional keys. Pulling the common shape into a helper will make future schema changes much cheaper and keep the tests focused on what each case is actually exercising.Also applies to: 122-132, 170-180, 230-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitMediaUploadTypeApiControllerTest.php` around lines 57 - 69, Extract the repeated payload creation into a single helper (e.g., mediaUploadTypePayload or buildMediaUploadTypePayload) inside the test class used by OAuth2SummitMediaUploadTypeApiControllerTest; have it return the common base array (name, type_id, description, max_size, is_mandatory, is_editable, private_storage_type, public_storage_type, presentation_types, min_uploads_qty, max_uploads_qty) and accept an optional overrides array to merge in test-specific keys. Replace the four inline payloads (the one around the shown diff and the ones at the other noted locations) with calls to this helper passing only differing keys, keeping the original keys and constant values in the helper so future schema changes are updated in one place.tests/OAuth2SummitPresentationActionTypeApiTest.php (1)
59-87: Check for the seeded action types, not just a minimum count.
$page->total >= 2is stable, but it no longer proves that the two action types created insetUp()are present in the response. With shared summit fixtures, this can still pass if unrelated rows fill the page. Please assert on the returned labels or ids forACTION1andACTION2.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitPresentationActionTypeApiTest.php` around lines 59 - 87, Replace the weak count assertion in testGetAllPerSummit with explicit checks that the seeded action types from setUp() are present: after decoding $content into $page (from the OAuth2SummitPresentationActionTypeApiController@getAllBySummit call), iterate/search $page->data (or the items array) for entries whose label or id matches the seeded ACTION1 and ACTION2 values used in setUp(), and assert both are found; this ensures the test looks for the exact seeded labels/ids rather than just a minimum total count.tests/OAuth2SummitOrdersApiTest.php (1)
103-111: Minor optimization: Redundant persist/flush of summit.The summit is already persisted and flushed on lines 100-101. The second
persist(self::$summit)on line 110 is unnecessary since the summit entity itself hasn't changed—only its related orders/tickets have been modified. You only need to flush the entity manager to persist the hash changes.♻️ Suggested simplification
// Generate hashes on orders and tickets for hash-dependent tests foreach (self::$summit->getOrders() as $order) { $order->generateHash(); foreach ($order->getTickets() as $ticket) { $ticket->generateHash(); } } - self::$em->persist(self::$summit); self::$em->flush();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitOrdersApiTest.php` around lines 103 - 111, Remove the redundant self::$em->persist(self::$summit) call after generating hashes on orders and tickets: the summit entity instance (self::$summit) itself wasn't replaced, only related Order and Ticket entities had their generateHash() invoked, so simply call self::$em->flush() to persist those changes. Update the block that iterates over self::$summit->getOrders() and ->getTickets() (and the generateHash() calls) to remove the extra self::$em->persist(self::$summit) while keeping self::$em->flush() to write the hash changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/OAuth2SummitOrdersApiTest.php`:
- Around line 1113-1121: In testDelegateAndUpdateEmail add defensive null checks
before using self::$summit->getOrders()[0] and $order->getFirstTicket(): verify
self::$summit is set and has at least one order, assign $order only after that
check, then verify $order is not null and has a ticket before assigning $ticket;
if any check fails, call the same test-skip or fail helper used elsewhere in the
suite (consistent with other delegation tests) so the test exits cleanly rather
than dereferencing null values.
- Around line 1001-1010: In testDoubleDelegate update the setup to safely handle
missing data by checking that self::$summit->getOrders() is non-empty and that
$order->getFirstTicket() is not null before proceeding; if no orders or no
ticket exist, call $this->markTestSkipped with a clear message. Locate the
testDoubleDelegate method and add guards around the retrieval of $order (from
self::$summit->getOrders()[0]) and $ticket (from $order->getFirstTicket()) to
avoid dereferencing null/empty values and ensure consistent behavior with other
tests like testUpdateTicket and testRevokeAttendee.
- Around line 1051-1060: testDelegateTicket accesses
self::$summit->getOrders()[0] and $order->getFirstTicket() without null/empty
checks; add the same defensive checks used in testDoubleDelegate: verify
self::$summit is set and has orders (check getOrders() is non-empty) before
using [0], then verify $order is not null and that getFirstTicket() returned a
ticket before proceeding (otherwise call $this->markTestSkipped or assertNotNull
and return) so the test won't throw on missing data.
- Around line 302-345: The testUpdateOrder method is using hardcoded
extra_questions question_id values (3,4,5) that aren't created by the test data;
update the test to either create the missing
SummitSelectionPlanExtraQuestionType entries via the InsertSummitTestData setup
and use their actual IDs, or remove extra_questions from the payload if they are
not required for this update scenario; specifically modify testUpdateOrder to
obtain real question IDs (e.g., by creating questions in the
InsertSummitTestData trait or querying self::$summit for its selection plan
questions) and replace the hardcoded IDs in the extra_questions array, or drop
the extra_questions key entirely.
In `@tests/OAuth2SummitPresentationActionTypeApiTest.php`:
- Around line 214-245: In testUpdateAction capture the expected label into a
local variable before making the PUT request (e.g. $expectedLabel =
self::$action2->getLabel()." UPDATE") so the test asserts against the payload
value rather than the possibly mutating EntityManager state; then change the
final assertion that currently compares $action->label to
self::$action2->getLabel() to assert $action->label equals that $expectedLabel
variable while keeping the other assertions (id, non-null) intact.
- Around line 148-179: The testReorderAction currently only checks the returned
entity id; update it to assert the reorder actually occurred by verifying the
returned object's order property (assert $action->order == 1) after the PUT to
OAuth2SummitPresentationActionTypeApiController@update, and optionally
load/inspect the other affected action (self::$action1/self::$action2) or the
summit's action list to confirm relative ordering changed as expected; ensure
the assertion uses the decoded $action from the response and keeps the existing
status check.
---
Nitpick comments:
In `@tests/OAuth2SummitMediaUploadTypeApiControllerTest.php`:
- Around line 57-69: Extract the repeated payload creation into a single helper
(e.g., mediaUploadTypePayload or buildMediaUploadTypePayload) inside the test
class used by OAuth2SummitMediaUploadTypeApiControllerTest; have it return the
common base array (name, type_id, description, max_size, is_mandatory,
is_editable, private_storage_type, public_storage_type, presentation_types,
min_uploads_qty, max_uploads_qty) and accept an optional overrides array to
merge in test-specific keys. Replace the four inline payloads (the one around
the shown diff and the ones at the other noted locations) with calls to this
helper passing only differing keys, keeping the original keys and constant
values in the helper so future schema changes are updated in one place.
In `@tests/OAuth2SummitOrdersApiTest.php`:
- Around line 103-111: Remove the redundant self::$em->persist(self::$summit)
call after generating hashes on orders and tickets: the summit entity instance
(self::$summit) itself wasn't replaced, only related Order and Ticket entities
had their generateHash() invoked, so simply call self::$em->flush() to persist
those changes. Update the block that iterates over self::$summit->getOrders()
and ->getTickets() (and the generateHash() calls) to remove the extra
self::$em->persist(self::$summit) while keeping self::$em->flush() to write the
hash changes.
In `@tests/OAuth2SummitPresentationActionTypeApiTest.php`:
- Around line 59-87: Replace the weak count assertion in testGetAllPerSummit
with explicit checks that the seeded action types from setUp() are present:
after decoding $content into $page (from the
OAuth2SummitPresentationActionTypeApiController@getAllBySummit call),
iterate/search $page->data (or the items array) for entries whose label or id
matches the seeded ACTION1 and ACTION2 values used in setUp(), and assert both
are found; this ensures the test looks for the exact seeded labels/ids rather
than just a minimum total count.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: aaeed0da-6514-4945-bffb-a2b4c696eaea
📒 Files selected for processing (8)
.github/workflows/push.ymltests/InsertSummitTestData.phptests/OAuth2SummitMediaUploadTypeApiControllerTest.phptests/OAuth2SummitNotificationsApiControllerTest.phptests/OAuth2SummitOrderExtraQuestionTypeApiTest.phptests/OAuth2SummitOrdersApiTest.phptests/OAuth2SummitPresentationActionApiTest.phptests/OAuth2SummitPresentationActionTypeApiTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/push.yml
OAuth2SummitOrders, MediaUpload, Notifications, OrderExtraQuestion, PresentationAction and PresentationActionType tests chore(ci): add OAuth2SummitMediaFileTypeApiControllerTest, OAuth2SummitMediaUploadTypeApiControllerTest, OAuth2SummitMetricsApiControllerTest, OAuth2SummitNotificationsApiControllerTest, OAuth2SummitOrderExtraQuestionTypeApiTest, OAuth2SummitOrdersApiTest, OAuth2SummitPresentationActionApiTest, OAuth2SummitPresentationActionTypeApiTest
smarcet
force-pushed
the
hotfix/unit-test-fixes
branch
from
1219099 to
8afa518
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/OAuth2SummitOrdersApiTest.php (3)
1001-1004:⚠️ Potential issue | 🟡 MinorInconsistent null guard pattern - potential undefined offset error.
testDoubleDelegate,testDelegateTicket, andtestDelegateAndUpdateEmailaccessgetOrders()[0]directly without null checks, unlike other tests that use->first()withmarkTestSkipped. If no orders exist, this will cause an "Undefined offset: 0" error rather than gracefully skipping.🛡️ Suggested fix for consistency
public function testDoubleDelegate(){ - $order = self::$summit->getOrders()[0]; + $order = self::$summit->getOrders()->first(); + if(is_null($order)) $this->markTestSkipped('No orders in test data'); $ticket = $order->getFirstTicket(); + if(is_null($ticket)) $this->markTestSkipped('No tickets in test data');Apply the same pattern to
testDelegateTicket(line 1053) andtestDelegateAndUpdateEmail(line 1114).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitOrdersApiTest.php` around lines 1001 - 1004, Replace direct array access getOrders()[0] in testDoubleDelegate, testDelegateTicket, and testDelegateAndUpdateEmail with the same null-guard pattern used elsewhere: retrieve orders via self::$summit->getOrders()->first() (or $orders = self::$summit->getOrders(); $first = $orders->first()), check if the result is null, and call $this->markTestSkipped('no orders available') when null before proceeding to use $order and $ticket; update references to getFirstTicket() accordingly so the tests gracefully skip instead of raising "Undefined offset: 0".
268-274:⚠️ Potential issue | 🟡 MinorHardcoded
question_idmay not exist in test data.The test payload references
question_id => 1, but since the test now uses dynamically created test data, there's no guarantee that an extra question with ID 1 exists. This could cause the test to fail with a validation error or be silently ignored.Consider either removing the
extra_questionsfrom the payload if not being tested, or creating the extra question in the test setup and referencing it dynamically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitOrdersApiTest.php` around lines 268 - 274, The payload in OAuth2SummitOrdersApiTest uses a hardcoded extra question id (in the $data array 'extra_questions' => [['question_id' => 1, ...]]), which may not exist with dynamic test fixtures; either remove the 'extra_questions' entry from $data if this test does not assert extra-question behavior, or create the extra question during setup (e.g., via the test helper or factory used elsewhere in the suite) and set 'question_id' to the created question's id variable before sending $data so the test references a real record.
317-321:⚠️ Potential issue | 🟡 MinorHardcoded
question_idvalues (3, 4, 5) may not exist in test data.Same issue as
testUpdateMyOrder- these hardcoded question IDs are unlikely to exist in the dynamically generated test data. This will likely cause test failures or unexpected behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitOrdersApiTest.php` around lines 317 - 321, The test uses hardcoded question_id values (3,4,5) in the 'extra_questions' payload which may not exist; instead create or retrieve actual question entities and use their IDs: e.g., create three registration questions (or fetch the first three from the summit/registration form) and assign their IDs to variables ($q1Id, $q2Id, $q3Id), then build 'extra_questions' using those IDs (replace ['question_id' => 3, ...] with ['question_id' => $q1Id, ...], etc. Update the test case in OAuth2SummitOrdersApiTest (same pattern as fix in testUpdateMyOrder) so it constructs questions programmatically and uses their real IDs in the payload.
♻️ Duplicate comments (1)
.github/workflows/push.yml (1)
72-125:⚠️ Potential issue | 🔴 CriticalPHPUnit
--filtervalues use file-path syntax—these will silently run zero tests.This issue was flagged in a previous review and remains unresolved. All newly added entries use path-like values (e.g.,
--filter test/OAuth2AttendeesApiTest,--filter tests/OAuth2SummitLocationsApiTest), but PHPUnit's--filteroption matches test class/method names by regex, not file paths. These will match nothing, causing the matrix jobs to pass without executing any tests.Additionally, there's inconsistency between
test/(lines 72-95) andtests/(lines 96-125).Fix: Remove the path prefix and use only the class name for each entry.
🔧 Proposed fix: correct the filter values
- - { name: "OAuth2AttendeesApiTest", filter: "--filter test/OAuth2AttendeesApiTest"} - - { name: "OAuth2SummitSponsorApiTest", filter: "--filter test/OAuth2SummitSponsorApiTest"} - - { name: "OAuth2BookableRoomAttributeTypesApiTest", filter: "--filter test/OAuth2BookableRoomAttributeTypesApiTest"} - - { name: "OAuth2CompaniesApiTest", filter: "--filter test/OAuth2CompaniesApiTest"} - - { name: "OAuth2ElectionsApiController", filter: "--filter test/OAuth2ElectionsApiController"} - - { name: "OAuth2MembersApiTest", filter: "--filter test/OAuth2MembersApiTest"} - - { name: "OAuth2GroupsApiTest", filter: "--filter test/OAuth2GroupsApiTest"} - - { name: "OAuth2OAuth2SponsorshipTypeApiTest", filter: "--filter test/OAuth2OAuth2SponsorshipTypeApiTest"} - - { name: "OAuth2OrganizationsApiTest", filter: "--filter test/OAuth2OrganizationsApiTest"} - - { name: "OAuth2PresentationApiTest", filter: "--filter test/OAuth2PresentationApiTest"} - - { name: "OAuth2PromoCodesApiTest", filter: "--filter test/OAuth2PromoCodesApiTest"} - - { name: "OAuth2SummitPromoCodesApiTest", filter: "--filter test/OAuth2SummitPromoCodesApiTest"} - - { name: "OAuth2SelectionPlansApiTest", filter: "--filter test/OAuth2SelectionPlansApiTest"} - - { name: "OAuth2SummitSpeakersApiTest", filter: "--filter test/OAuth2SummitSpeakersApiTest" } - - { name: "OAuth2SummitBadgesApiTest", filter: "--filter test/OAuth2SummitBadgesApiTest" } - - { name: "OAuth2TracksApiTest", filter: "--filter test/OAuth2TracksApiTest" } - - { name: "OAuth2TrackTagGroupsApiTest", filter: "--filter test/OAuth2TrackTagGroupsApiTest" } - - { name: "OAuth2SummitTracksApiTest", filter: "--filter test/OAuth2SummitTracksApiTest" } - - { name: "OAuth2TagsApiTest", filter: "--filter test/OAuth2TagsApiTest" } - - { name: "OAuth2SummitSponsorshipApiControllerTest", filter: "--filter test/OAuth2SummitSponsorshipApiControllerTest" } - - { name: "OAuth2SummitSponsorshipTypeApiControllerTest", filter: "--filter test/OAuth2SummitSponsorshipTypeApiControllerTest" } - - { name: "OAuth2SummitSubmissionInvitationApiTest", filter: "--filter test/OAuth2SummitSubmissionInvitationApiTest" } - - { name: "OAuth2SummitSubmittersApiTest", filter: "--filter test/OAuth2SummitSubmittersApiTest" } - - { name: "OAuth2SummitTaxTypeApiTest", filter: "--filter test/OAuth2SummitTaxTypeApiTest" } - - { name: "OAuth2SummitTicketTypesApiTest", filter: "--filter tests/OAuth2SummitTicketTypesApiTest" } - - { name: "OAuth2SummitTicketsApiTest", filter: "--filter tests/OAuth2SummitTicketsApiTest" } - - { name: "OAuth2SummitTrackChairsApiTest", filter: "--filter tests/OAuth2SummitTrackChairsApiTest" } - - { name: "OAuth2SummitTrackChairsRankingApiTest", filter: "--filter tests/OAuth2SummitTrackChairsRankingApiTest" } - - { name: "OAuth2TrackGroupsApiTest", filter: "--filter tests/OAuth2TrackGroupsApiTest" } - - { name: "OAuth2TrackQuestionsTemplateTest", filter: "--filter tests/OAuth2TrackQuestionsTemplateTest" } - - { name: "OAuth2SummitBadgeTypeApiTest", filter: "--filter tests/OAuth2SummitBadgeTypeApiTest" } - - { name: "OAuth2SummitBadgeViewTypeApiTest", filter: "--filter tests/OAuth2SummitBadgeViewTypeApiTest" } - - { name: "OAuth2SummitDocumentsApiControllerTest", filter: "--filter tests/OAuth2SummitDocumentsApiControllerTest" } - - { name: "OAuth2SummitEmailEventFlowApiControllerTest", filter: "--filter tests/OAuth2SummitEmailEventFlowApiControllerTest" } - - { name: "OAuth2SummitEventsBulkActionsTest", filter: "--filter tests/OAuth2SummitEventsBulkActionsTest" } - - { name: "OAuth2SummitLocationsApiTest", filter: "--filter tests/OAuth2SummitLocationsApiTest" } - - { name: "OAuth2RSVPSummitEventApiTest", filter: "--filter tests/OAuth2RSVPSummitEventApiTest" } - - { name: "OAuth2SpeakerActiveInvolvementApiTest", filter: "--filter tests/OAuth2SpeakerActiveInvolvementApiTest" } - - { name: "OAuth2SpeakerOrganizationalRoleApiTest", filter: "--filter tests/OAuth2SpeakerOrganizationalRoleApiTest" } - - { name: "OAuth2SpeakersAssistancesApiTest", filter: "--filter tests/OAuth2SpeakersAssistancesApiTest" } - - { name: "OAuth2SponsoredProjectsApiTest", filter: "--filter tests/OAuth2SponsoredProjectsApiTest" } - - { name: "OAuth2SummitAccessLevelTypeTest", filter: "--filter tests/OAuth2SummitAccessLevelTypeTest" } - - { name: "OAuth2SummitAdministratorPermissionGroupApiControllerTest", filter: "--filter tests/OAuth2SummitAdministratorPermissionGroupApiControllerTest" } - - { name: "OAuth2SummitAttendeeBadgePrintApiControllerTest", filter: "--filter tests/OAuth2SummitAttendeeBadgePrintApiControllerTest" } - - { name: "OAuth2SummitAttendeeNotesApiTest", filter: "--filter tests/OAuth2SummitAttendeeNotesApiTest" } - - { name: "OAuth2SummitBadgeFeatureTypeApiTest", filter: "--filter tests/OAuth2SummitBadgeFeatureTypeApiTest" } - - { name: "OAuth2SummitMediaFileTypeApiControllerTest", filter: "--filter tests/OAuth2SummitMediaFileTypeApiControllerTest" } - - { name: "OAuth2SummitMediaUploadTypeApiControllerTest", filter: "--filter tests/OAuth2SummitMediaUploadTypeApiControllerTest" } - - { name: "OAuth2SummitMetricsApiControllerTest", filter: "--filter tests/OAuth2SummitMetricsApiControllerTest" } - - { name: "OAuth2SummitNotificationsApiControllerTest", filter: "--filter tests/OAuth2SummitNotificationsApiControllerTest" } - - { name: "OAuth2SummitOrderExtraQuestionTypeApiTest", filter: "--filter tests/OAuth2SummitOrderExtraQuestionTypeApiTest" } - - { name: "OAuth2SummitOrdersApiTest", filter: "--filter tests/OAuth2SummitOrdersApiTest" } - - { name: "OAuth2SummitPresentationActionApiTest", filter: "--filter tests/OAuth2SummitPresentationActionApiTest" } - - { name: "OAuth2SummitPresentationActionTypeApiTest", filter: "--filter tests/OAuth2SummitPresentationActionTypeApiTest" } + - { name: "OAuth2AttendeesApiTest", filter: "--filter OAuth2AttendeesApiTest" } + - { name: "OAuth2SummitSponsorApiTest", filter: "--filter OAuth2SummitSponsorApiTest" } + - { name: "OAuth2BookableRoomAttributeTypesApiTest", filter: "--filter OAuth2BookableRoomAttributeTypesApiTest" } + - { name: "OAuth2CompaniesApiTest", filter: "--filter OAuth2CompaniesApiTest" } + - { name: "OAuth2ElectionsApiController", filter: "--filter OAuth2ElectionsApiController" } + - { name: "OAuth2MembersApiTest", filter: "--filter OAuth2MembersApiTest" } + - { name: "OAuth2GroupsApiTest", filter: "--filter OAuth2GroupsApiTest" } + - { name: "OAuth2OAuth2SponsorshipTypeApiTest", filter: "--filter OAuth2OAuth2SponsorshipTypeApiTest" } + - { name: "OAuth2OrganizationsApiTest", filter: "--filter OAuth2OrganizationsApiTest" } + - { name: "OAuth2PresentationApiTest", filter: "--filter OAuth2PresentationApiTest" } + - { name: "OAuth2PromoCodesApiTest", filter: "--filter OAuth2PromoCodesApiTest" } + - { name: "OAuth2SummitPromoCodesApiTest", filter: "--filter OAuth2SummitPromoCodesApiTest" } + - { name: "OAuth2SelectionPlansApiTest", filter: "--filter OAuth2SelectionPlansApiTest" } + - { name: "OAuth2SummitSpeakersApiTest", filter: "--filter OAuth2SummitSpeakersApiTest" } + - { name: "OAuth2SummitBadgesApiTest", filter: "--filter OAuth2SummitBadgesApiTest" } + - { name: "OAuth2TracksApiTest", filter: "--filter OAuth2TracksApiTest" } + - { name: "OAuth2TrackTagGroupsApiTest", filter: "--filter OAuth2TrackTagGroupsApiTest" } + - { name: "OAuth2SummitTracksApiTest", filter: "--filter OAuth2SummitTracksApiTest" } + - { name: "OAuth2TagsApiTest", filter: "--filter OAuth2TagsApiTest" } + - { name: "OAuth2SummitSponsorshipApiControllerTest", filter: "--filter OAuth2SummitSponsorshipApiControllerTest" } + - { name: "OAuth2SummitSponsorshipTypeApiControllerTest", filter: "--filter OAuth2SummitSponsorshipTypeApiControllerTest" } + - { name: "OAuth2SummitSubmissionInvitationApiTest", filter: "--filter OAuth2SummitSubmissionInvitationApiTest" } + - { name: "OAuth2SummitSubmittersApiTest", filter: "--filter OAuth2SummitSubmittersApiTest" } + - { name: "OAuth2SummitTaxTypeApiTest", filter: "--filter OAuth2SummitTaxTypeApiTest" } + - { name: "OAuth2SummitTicketTypesApiTest", filter: "--filter OAuth2SummitTicketTypesApiTest" } + - { name: "OAuth2SummitTicketsApiTest", filter: "--filter OAuth2SummitTicketsApiTest" } + - { name: "OAuth2SummitTrackChairsApiTest", filter: "--filter OAuth2SummitTrackChairsApiTest" } + - { name: "OAuth2SummitTrackChairsRankingApiTest", filter: "--filter OAuth2SummitTrackChairsRankingApiTest" } + - { name: "OAuth2TrackGroupsApiTest", filter: "--filter OAuth2TrackGroupsApiTest" } + - { name: "OAuth2TrackQuestionsTemplateTest", filter: "--filter OAuth2TrackQuestionsTemplateTest" } + - { name: "OAuth2SummitBadgeTypeApiTest", filter: "--filter OAuth2SummitBadgeTypeApiTest" } + - { name: "OAuth2SummitBadgeViewTypeApiTest", filter: "--filter OAuth2SummitBadgeViewTypeApiTest" } + - { name: "OAuth2SummitDocumentsApiControllerTest", filter: "--filter OAuth2SummitDocumentsApiControllerTest" } + - { name: "OAuth2SummitEmailEventFlowApiControllerTest", filter: "--filter OAuth2SummitEmailEventFlowApiControllerTest" } + - { name: "OAuth2SummitEventsBulkActionsTest", filter: "--filter OAuth2SummitEventsBulkActionsTest" } + - { name: "OAuth2SummitLocationsApiTest", filter: "--filter OAuth2SummitLocationsApiTest" } + - { name: "OAuth2RSVPSummitEventApiTest", filter: "--filter OAuth2RSVPSummitEventApiTest" } + - { name: "OAuth2SpeakerActiveInvolvementApiTest", filter: "--filter OAuth2SpeakerActiveInvolvementApiTest" } + - { name: "OAuth2SpeakerOrganizationalRoleApiTest", filter: "--filter OAuth2SpeakerOrganizationalRoleApiTest" } + - { name: "OAuth2SpeakersAssistancesApiTest", filter: "--filter OAuth2SpeakersAssistancesApiTest" } + - { name: "OAuth2SponsoredProjectsApiTest", filter: "--filter OAuth2SponsoredProjectsApiTest" } + - { name: "OAuth2SummitAccessLevelTypeTest", filter: "--filter OAuth2SummitAccessLevelTypeTest" } + - { name: "OAuth2SummitAdministratorPermissionGroupApiControllerTest", filter: "--filter OAuth2SummitAdministratorPermissionGroupApiControllerTest" } + - { name: "OAuth2SummitAttendeeBadgePrintApiControllerTest", filter: "--filter OAuth2SummitAttendeeBadgePrintApiControllerTest" } + - { name: "OAuth2SummitAttendeeNotesApiTest", filter: "--filter OAuth2SummitAttendeeNotesApiTest" } + - { name: "OAuth2SummitBadgeFeatureTypeApiTest", filter: "--filter OAuth2SummitBadgeFeatureTypeApiTest" } + - { name: "OAuth2SummitMediaFileTypeApiControllerTest", filter: "--filter OAuth2SummitMediaFileTypeApiControllerTest" } + - { name: "OAuth2SummitMediaUploadTypeApiControllerTest", filter: "--filter OAuth2SummitMediaUploadTypeApiControllerTest" } + - { name: "OAuth2SummitMetricsApiControllerTest", filter: "--filter OAuth2SummitMetricsApiControllerTest" } + - { name: "OAuth2SummitNotificationsApiControllerTest", filter: "--filter OAuth2SummitNotificationsApiControllerTest" } + - { name: "OAuth2SummitOrderExtraQuestionTypeApiTest", filter: "--filter OAuth2SummitOrderExtraQuestionTypeApiTest" } + - { name: "OAuth2SummitOrdersApiTest", filter: "--filter OAuth2SummitOrdersApiTest" } + - { name: "OAuth2SummitPresentationActionApiTest", filter: "--filter OAuth2SummitPresentationActionApiTest" } + - { name: "OAuth2SummitPresentationActionTypeApiTest", filter: "--filter OAuth2SummitPresentationActionTypeApiTest" }Note: The pre-existing entries at lines 67 and 71 also have this issue:
- { name: "EntityModelUnitTests", filter: "tests/Unit/Entities/" } - { name: "Repositories", filter: "--filter tests/Repositories/" }Consider fixing these as well by either using class name patterns or passing the directory as a PHPUnit argument instead of via
--filter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/push.yml around lines 72 - 125, The matrix entries use PHPUnit --filter with file-path-like values which match nothing; update each filter for test names such as OAuth2AttendeesApiTest and OAuth2SummitLocationsApiTest to supply only the test class name (e.g. "--filter OAuth2AttendeesApiTest") instead of "test/..." or "tests/...", and normalize the inconsistent prefixes (remove both "test/" and "tests/"); also fix the pre-existing entries EntityModelUnitTests and Repositories by either replacing their path filters with class-name patterns or moving directory selection to a proper PHPUnit argument (not --filter).
🧹 Nitpick comments (4)
tests/OAuth2SummitOrdersApiTest.php (1)
103-111: Redundant persist call.After generating hashes, calling
persist(self::$summit)again is unnecessary since the summit entity is already managed by the EntityManager from the earlier persist on line 100. Onlyflush()is needed to persist the hash changes.♻️ Suggested simplification
// Generate hashes on orders and tickets for hash-dependent tests foreach (self::$summit->getOrders() as $order) { $order->generateHash(); foreach ($order->getTickets() as $ticket) { $ticket->generateHash(); } } - self::$em->persist(self::$summit); self::$em->flush();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitOrdersApiTest.php` around lines 103 - 111, The call to self::$em->persist(self::$summit) after generating hashes is redundant because self::$summit is already managed; remove the unnecessary self::$em->persist(self::$summit) line and leave only the self::$em->flush() so that generateHash() changes on orders and tickets are flushed to the database (locate the loop that calls order->generateHash() and ticket->generateHash() and the subsequent self::$em->persist/self::$em->flush block).tests/OAuth2SummitNotificationsApiControllerTest.php (3)
136-138: Strengthen response assertions beyond non-null payload checks.At Line [136]-Line [138], Line [167]-Line [169], and Line [240]-Line [242], asserting only non-null can miss schema regressions. Consider asserting key fields like
id,channel, and expected IDs/messages.Also applies to: 167-169, 240-242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitNotificationsApiControllerTest.php` around lines 136 - 138, Replace the weak non-null checks on the decoded response ($notification = json_decode($content)) with stronger assertions: verify required fields exist and have expected types/values (for example assertNotNull($notification->id), assertIsString($notification->channel) or assertNotEmpty($notification->message), and where applicable assertEquals(expectedId, $notification->id) or assertEquals(expectedChannel, $notification->channel)); update each occurrence that returns $notification to include these assertions so schema regressions are caught (references: $notification, json_decode($content), assertTrue usage in the test methods).
141-143: Avoid calling one test from another (Line [142]).
testGetPushNotificationByIddirectly invokestestAddPushNotificationEveryone(). This couples tests and obscures root-cause failures. Extract shared setup into a private helper and keep test methodsvoid.♻️ Refactor sketch
+ private function createPushNotificationEveryone(): object + { + $params = ['id' => self::$summit->getId()]; + $message = \Illuminate\Support\Str::random(16) . '_message'; + $data = [ + 'message' => $message, + 'channel' => \models\summit\SummitPushNotificationChannel::Everyone, + 'platform' => \models\summit\SummitPushNotification::PlatformMobile, + ]; + $headers = [ + "HTTP_Authorization" => " Bearer " . $this->access_token, + "CONTENT_TYPE" => "application/json", + ]; + $response = $this->action("POST", "OAuth2SummitNotificationsApiController@addPushNotification", $params, [], [], [], $headers, json_encode($data)); + $this->assertResponseStatus(201); + $notification = json_decode($response->getContent()); + $this->assertNotNull($notification); + return $notification; + } + - public function testAddPushNotificationEveryone(){ - ... - return $notification; + public function testAddPushNotificationEveryone(): void { + $this->createPushNotificationEveryone(); } - public function testGetPushNotificationById(){ - $notification = $this->testAddPushNotificationEveryone(); + public function testGetPushNotificationById(): void { + $notification = $this->createPushNotificationEveryone(); ... }You can verify whether this anti-pattern appears elsewhere with:
#!/bin/bash set -euo pipefail rg -nP --type php -C2 '\$this->test[A-Z][A-Za-z0-9_]*\s*\(' tests🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitNotificationsApiControllerTest.php` around lines 141 - 143, testGetPushNotificationById currently calls another test method (testAddPushNotificationEveryone); extract the shared setup into a private helper (e.g., createPushNotificationEveryone or setUpPushNotificationEveryone) that returns the created notification, then update testAddPushNotificationEveryone and testGetPushNotificationById to call that helper instead of calling each other so both test methods remain independent and void; reference the existing methods testGetPushNotificationById and testAddPushNotificationEveryone when locating where to move the setup logic.
110-110: Use\Illuminate\Support\Str::random()instead of the globalstr_random()helper for better code clarity and framework compatibility.While
str_randomis available through the installedlaravel/helperspackage, using the explicit\Illuminate\Support\Str::random()method is the recommended approach in Laravel 12. This avoids reliance on implicit global helper function loading and is more maintainable across environments.Replace at lines 110, 179, and 213 in
tests/OAuth2SummitNotificationsApiControllerTest.php(and all other occurrences throughout the codebase):Refactoring locations
tests/OAuth2SummitNotificationsApiControllerTest.php: Lines 110, 179, 213
app/Services/Apis/PasswordlessAPI.php: Line 72Plus 45+ additional locations across test files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitNotificationsApiControllerTest.php` at line 110, Replace all uses of the global helper str_random(...) with the framework-recommended Str::random(...) to avoid implicit helper reliance; specifically update occurrences in tests/OAuth2SummitNotificationsApiControllerTest.php (previous uses like $message = str_random(16).'_message' at the noted lines) and in app/Services/Apis/PasswordlessAPI.php (around the method using str_random at line 72) and all other instances across the codebase; either add a top-file import use Illuminate\Support\Str; and call Str::random(16) or use the fully-qualified \Illuminate\Support\Str::random(16) wherever str_random(...) is currently used.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/OAuth2SummitOrdersApiTest.php`:
- Around line 1001-1004: Replace direct array access getOrders()[0] in
testDoubleDelegate, testDelegateTicket, and testDelegateAndUpdateEmail with the
same null-guard pattern used elsewhere: retrieve orders via
self::$summit->getOrders()->first() (or $orders = self::$summit->getOrders();
$first = $orders->first()), check if the result is null, and call
$this->markTestSkipped('no orders available') when null before proceeding to use
$order and $ticket; update references to getFirstTicket() accordingly so the
tests gracefully skip instead of raising "Undefined offset: 0".
- Around line 268-274: The payload in OAuth2SummitOrdersApiTest uses a hardcoded
extra question id (in the $data array 'extra_questions' => [['question_id' => 1,
...]]), which may not exist with dynamic test fixtures; either remove the
'extra_questions' entry from $data if this test does not assert extra-question
behavior, or create the extra question during setup (e.g., via the test helper
or factory used elsewhere in the suite) and set 'question_id' to the created
question's id variable before sending $data so the test references a real
record.
- Around line 317-321: The test uses hardcoded question_id values (3,4,5) in the
'extra_questions' payload which may not exist; instead create or retrieve actual
question entities and use their IDs: e.g., create three registration questions
(or fetch the first three from the summit/registration form) and assign their
IDs to variables ($q1Id, $q2Id, $q3Id), then build 'extra_questions' using those
IDs (replace ['question_id' => 3, ...] with ['question_id' => $q1Id, ...], etc.
Update the test case in OAuth2SummitOrdersApiTest (same pattern as fix in
testUpdateMyOrder) so it constructs questions programmatically and uses their
real IDs in the payload.
---
Duplicate comments:
In @.github/workflows/push.yml:
- Around line 72-125: The matrix entries use PHPUnit --filter with
file-path-like values which match nothing; update each filter for test names
such as OAuth2AttendeesApiTest and OAuth2SummitLocationsApiTest to supply only
the test class name (e.g. "--filter OAuth2AttendeesApiTest") instead of
"test/..." or "tests/...", and normalize the inconsistent prefixes (remove both
"test/" and "tests/"); also fix the pre-existing entries EntityModelUnitTests
and Repositories by either replacing their path filters with class-name patterns
or moving directory selection to a proper PHPUnit argument (not --filter).
---
Nitpick comments:
In `@tests/OAuth2SummitNotificationsApiControllerTest.php`:
- Around line 136-138: Replace the weak non-null checks on the decoded response
($notification = json_decode($content)) with stronger assertions: verify
required fields exist and have expected types/values (for example
assertNotNull($notification->id), assertIsString($notification->channel) or
assertNotEmpty($notification->message), and where applicable
assertEquals(expectedId, $notification->id) or assertEquals(expectedChannel,
$notification->channel)); update each occurrence that returns $notification to
include these assertions so schema regressions are caught (references:
$notification, json_decode($content), assertTrue usage in the test methods).
- Around line 141-143: testGetPushNotificationById currently calls another test
method (testAddPushNotificationEveryone); extract the shared setup into a
private helper (e.g., createPushNotificationEveryone or
setUpPushNotificationEveryone) that returns the created notification, then
update testAddPushNotificationEveryone and testGetPushNotificationById to call
that helper instead of calling each other so both test methods remain
independent and void; reference the existing methods testGetPushNotificationById
and testAddPushNotificationEveryone when locating where to move the setup logic.
- Line 110: Replace all uses of the global helper str_random(...) with the
framework-recommended Str::random(...) to avoid implicit helper reliance;
specifically update occurrences in
tests/OAuth2SummitNotificationsApiControllerTest.php (previous uses like
$message = str_random(16).'_message' at the noted lines) and in
app/Services/Apis/PasswordlessAPI.php (around the method using str_random at
line 72) and all other instances across the codebase; either add a top-file
import use Illuminate\Support\Str; and call Str::random(16) or use the
fully-qualified \Illuminate\Support\Str::random(16) wherever str_random(...) is
currently used.
In `@tests/OAuth2SummitOrdersApiTest.php`:
- Around line 103-111: The call to self::$em->persist(self::$summit) after
generating hashes is redundant because self::$summit is already managed; remove
the unnecessary self::$em->persist(self::$summit) line and leave only the
self::$em->flush() so that generateHash() changes on orders and tickets are
flushed to the database (locate the loop that calls order->generateHash() and
ticket->generateHash() and the subsequent self::$em->persist/self::$em->flush
block).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a0883ec6-1313-4836-9969-950a201013af
📒 Files selected for processing (8)
.github/workflows/push.ymltests/InsertSummitTestData.phptests/OAuth2SummitMediaUploadTypeApiControllerTest.phptests/OAuth2SummitNotificationsApiControllerTest.phptests/OAuth2SummitOrderExtraQuestionTypeApiTest.phptests/OAuth2SummitOrdersApiTest.phptests/OAuth2SummitPresentationActionApiTest.phptests/OAuth2SummitPresentationActionTypeApiTest.php
✅ Files skipped from review due to trivial changes (1)
- tests/OAuth2SummitOrderExtraQuestionTypeApiTest.php
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/OAuth2SummitPresentationActionApiTest.php
- tests/InsertSummitTestData.php
chore(ci): add missing unit tests
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/OAuth2SummitRSVPTemplateApiTest.php (1)
211-239:⚠️ Potential issue | 🟡 MinorProve the delete actually removed the resource.
A bare
204can still be a false positive if the controller returns success without deleting anything. Re-fetch the template/question/value after the delete and expect the not-found response.Also applies to: 389-418, 505-535
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitRSVPTemplateApiTest.php` around lines 211 - 239, After performing the DELETE in testDeleteRSVPTemplate, re-fetch the same resource to ensure it was actually removed: call the GET action (e.g., use "GET" to "OAuth2SummitRSVPTemplatesApiController@getRSVPTemplate" with the same id/template_id headers/params) and assert a 404 (or appropriate not-found) response; apply the same pattern to the other delete tests referenced (the delete tests around lines 389-418 and 505-535) so each delete is followed by an explicit GET/assert-not-found check to prevent false positives.
♻️ Duplicate comments (1)
.github/workflows/push.yml (1)
73-136:⚠️ Potential issue | 🟠 MajorFix path-like PHPUnit filters; these suites may not execute the intended tests.
Many new entries use
--filter test/...or--filter tests/..., which are file-path-like patterns.--filteris for test-name regex matching, so these rows can pass without running the target suites.Suggested normalization pattern
- - { name: "OAuth2AttendeesApiTest", filter: "--filter test/OAuth2AttendeesApiTest"} + - { name: "OAuth2AttendeesApiTest", filter: "--filter OAuth2AttendeesApiTest" } - - { name: "OAuth2SummitTicketTypesApiTest", filter: "--filter tests/OAuth2SummitTicketTypesApiTest" } + - { name: "OAuth2SummitTicketTypesApiTest", filter: "--filter OAuth2SummitTicketTypesApiTest" }Apply the same normalization across all affected rows in this block.
#!/bin/bash set -euo pipefail echo "Rows using path-like values inside --filter:" python - <<'PY' from pathlib import Path import re p = Path(".github/workflows/push.yml") for i, line in enumerate(p.read_text().splitlines(), 1): if re.search(r'--filter\s+tests?/', line): print(f"{i}: {line.strip()}") PY echo echo "Candidate OAuth2 test files in repository:" fd -t f 'OAuth2.*Test\.php' tests | sort🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/push.yml around lines 73 - 136, The workflow uses path-like PHPUnit --filter values (e.g. entries for OAuth2SummitTicketTypesApiTest, OAuth2SummitTicketsApiTest, OAuth2TrackGroupsApiTest, etc.) which won't match test names; update each list item so the --filter is a test-name regex (not a file path). For every item where filter contains "test/" or "tests/", replace the filter value with the corresponding test class name from the name field (e.g. --filter OAuth2SummitTicketTypesApiTest) or a proper regex that matches the PHPUnit test name, and apply this normalization consistently across all entries in the affected block.
🧹 Nitpick comments (15)
tests/OAuth2SummitSelectionPlanExtraQuestionTypeApiTest.php (1)
94-95: Consider using idiomatic PHPUnit assertions for clearer failure messages.PHPUnit provides dedicated assertion methods that produce more descriptive error output on failure.
♻️ Suggested improvement
- $this->assertTrue(!is_null($page)); - $this->assertTrue($page->total >= 20); + $this->assertNotNull($page); + $this->assertGreaterThanOrEqual(20, $page->total);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitSelectionPlanExtraQuestionTypeApiTest.php` around lines 94 - 95, Replace the generic boolean assertions in the OAuth2SummitSelectionPlanExtraQuestionTypeApiTest test with PHPUnit's idiomatic assertions: use assertNotNull($page) instead of assertTrue(!is_null($page)) and use assertGreaterThanOrEqual(20, $page->total) (or assertGreaterThan(19, $page->total)) instead of assertTrue($page->total >= 20) so failures show clearer messages and expected vs actual values.tests/OAuth2SummitProposedScheduleApiTest.php (1)
151-183: Skipped test should be tracked for future resolution.The skip is reasonable given the data constraint, but indefinitely skipped tests reduce coverage. Consider adding a TODO comment with a reference to a tracking issue, or adjusting the test data setup to avoid the blackout collision so this test can run.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitProposedScheduleApiTest.php` around lines 151 - 183, The test method testPublishAll is being unconditionally skipped which hides coverage; either add a TODO comment in testPublishAll referencing a tracking ticket/issue ID and why it's skipped, or update the test setup so testPublishScheduleEvent and its generated presentations do not produce overlapping timeframes (adjust created presentations' start/end times or blackout rules) so publishAll (controller method publishAll) can run; ensure the TODO references testPublishAll and the related test helper testPublishScheduleEvent so the skip is tracked or remove the skip after fixing the test data.tests/OAuth2SummitScheduleSettingsApiTest.php (2)
709-717: Remove unused$responseassignments in request-only assertions.At Line 709 and Line 747,
$responseis assigned but never read. Since assertions rely onassertResponseStatus(...), you can drop the variable to reduce noise.♻️ Proposed cleanup
- $response = $this->action( + $this->action( "GET", "OAuth2SummitScheduleSettingsApiController@get", $params, [], [], [], $headers ); @@ - $response = $this->action( + $this->action( "DELETE", "OAuth2SummitScheduleSettingsApiController@delete", $params, [], [], [], $headers );Also applies to: 747-755
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitScheduleSettingsApiTest.php` around lines 709 - 717, Remove the unused $response assignments where the test only performs a status assertion: replace the assignments like $response = $this->action("GET", "OAuth2SummitScheduleSettingsApiController@get", ...) with a direct call to $this->action(...) (no assignment) in both locations (the GET to OAuth2SummitScheduleSettingsApiController@get and the other similar call around lines 747-755), leaving the subsequent assertResponseStatus(...) checks unchanged.
722-732: Skipped tests should be tracked to avoid silent coverage gaps.These new tests are currently always skipped, so the added scenarios are not enforced yet. Please tie each skip reason to a concrete tracker ID so they don’t become permanent blind spots.
I can draft follow-up issue templates for each skipped test path if you want.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitScheduleSettingsApiTest.php` around lines 722 - 732, Update the skipped tests to reference concrete tracker IDs so they are discoverable: replace the current markTestSkipped messages in testGetMetadata, testAddFilterToConfig, and testUpdateFilterById with messages that include the corresponding issue/bug/SONAR/TRELLO/JIRA IDs (e.g. "SKIP: issue XYZ-123 - Controller getMetadata() is unimplemented"), and add a TODO annotation above each test linking the same ID; if no issue exists, create one for each (Controller: getMetadata, Service: SummitScheduleSettingsService::addFilter, Service: SummitScheduleSettingsService::updateFilter) and use that new ID in the skip message so CI and reviewers can track remediation.tests/OAuth2SummitRegistrationCompaniesApiTest.php (3)
98-99: Unused variable$content.The variable is assigned but never used. Either remove it or add an assertion on the response content.
🧹 Proposed fix
- $content = $response->getContent(); $this->assertResponseStatus(201);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitRegistrationCompaniesApiTest.php` around lines 98 - 99, The test assigns $content = $response->getContent() but never uses it; either remove that unused assignment or add an assertion that validates the response body (for example assertNotEmpty($content) or an appropriate JSON/assertion) after the existing $this->assertResponseStatus(201) in OAuth2SummitRegistrationCompaniesApiTest (locate the $content = $response->getContent() line and update it accordingly).
135-137: Consider adding queue assertions.
Queue::fake()is correctly used for isolation, but withoutQueue::assertPushed()orQueue::assertNothingPushed(), the test doesn't verify whether the expected job was actually dispatched. If the import endpoint queues a job, asserting on it strengthens the test.💡 Example addition
// After assertResponseStatus(200), add: Queue::assertPushed(\App\Jobs\YourExpectedJob::class); // or if no job is expected: Queue::assertNothingPushed();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitRegistrationCompaniesApiTest.php` around lines 135 - 137, The test uses Storage::fake('swift') and Queue::fake() but never verifies queued jobs; after the existing assertResponseStatus(200) in OAuth2SummitRegistrationCompaniesApiTest you should add a queue assertion: if the import endpoint is supposed to dispatch a job, call Queue::assertPushed(YourExpectedJob::class) (use the actual job class name dispatched by the import flow), otherwise call Queue::assertNothingPushed() to assert no job was queued; place the assertion immediately after the response assertion so it validates dispatch behavior for the import endpoint.
129-130: Unused variable$content.Same as above—
$contentis assigned but not used.🧹 Proposed fix
- $content = $response->getContent(); $this->assertResponseStatus(204);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitRegistrationCompaniesApiTest.php` around lines 129 - 130, Remove the unused assignment to $content in OAuth2SummitRegistrationCompaniesApiTest.php: the line calling $response->getContent() and assigning to $content should be deleted (or replaced with an assertion that actually checks the content if intended); locate the code around the test where $content = $response->getContent(); and keep the existing $this->assertResponseStatus(204) call, removing the unused $content variable to eliminate the dead assignment.tests/OAuth2SummitProposedScheduleAllowedLocationApiControllerTest.php (2)
312-322: Unused$responsevariable.The
$responsevariable is assigned but never used. Per static analysis hint, consider either removing the assignment or prefixing with underscore to indicate intentional non-use.✏️ Suggested fix
- $response = $this->action( + $this->action( "DELETE", "OAuth2SummitProposedScheduleAllowedLocationApiController@removeAllowedLocationFromTrack", $params,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitProposedScheduleAllowedLocationApiControllerTest.php` around lines 312 - 322, The test assigns the result of the DELETE action to an unused $response variable in the OAuth2SummitProposedScheduleAllowedLocationApiController@removeAllowedLocationFromTrack call; remove the assignment or prefix the variable with an underscore (e.g., $_response) to signal intentional non-use so static analysis stops complaining, leaving the action invocation and subsequent $this->assertResponseStatus(204) intact in OAuth2SummitProposedScheduleAllowedLocationApiControllerTest.
385-395: Unused$responsevariable.Same issue as line 312: the
$responsevariable is assigned but never used.✏️ Suggested fix
- $response = $this->action( + $this->action( "DELETE", "OAuth2SummitProposedScheduleAllowedLocationApiController@removeTimeFrameFromAllowedLocation", $params,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitProposedScheduleAllowedLocationApiControllerTest.php` around lines 385 - 395, The test assigns the result of $this->action(...) to an unused $response variable; remove the unnecessary assignment and just call $this->action(...) (or, if you intended to inspect the response, use $response in assertions) for the call to OAuth2SummitProposedScheduleAllowedLocationApiController@removeTimeFrameFromAllowedLocation so that the code no longer creates an unused $response variable before assertResponseStatus(204).tests/OAuth2SummitRefundPolicyTypeApiTest.php (3)
210-211: Minor: Unused$contentvariable.The
$contentvariable is retrieved but never used. Since HTTP 204 (No Content) responses have no body by design, this line can be removed.♻️ Suggested fix
- $content = $response->getContent(); $this->assertResponseStatus(204);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitRefundPolicyTypeApiTest.php` around lines 210 - 211, Remove the unused $content variable retrieval in the test: delete the line that assigns $content = $response->getContent(); in the OAuth2SummitRefundPolicyTypeApiTest test method (the block that follows the $response handling and before $this->assertResponseStatus(204)); leave only the assertion $this->assertResponseStatus(204) since a 204 response has no body.
225-235: Minor: Unused$responsevariable (same as line 136).The
$responsevariable is assigned but not used directly.♻️ Suggested fix
- $response = $this->action( + $this->action( "DELETE", "OAuth2SummitRefundPolicyTypeApiController@delete", $params, [], [], [], $headers );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitRefundPolicyTypeApiTest.php` around lines 225 - 235, Remove the unused $response variable assignment from the DELETE test call: instead of assigning the result of $this->action(...) to $response, call $this->action(...) directly since the test only asserts the response status; update the test in OAuth2SummitRefundPolicyTypeApiTest.php where the DELETE call targets "OAuth2SummitRefundPolicyTypeApiController@delete" to eliminate the unused $response variable.
136-146: Minor: Unused$responsevariable.The
$responsevariable is assigned but not used. SinceassertResponseStatuschecks internal state, you can either remove the assignment or explicitly use$response->assertStatus(404)for clarity.♻️ Suggested fix
- $response = $this->action( + $this->action( "GET", "OAuth2SummitRefundPolicyTypeApiController@get", $params, [], [], [], $headers ); - - $this->assertResponseStatus(404); + + $this->assertResponseStatus(404);Or alternatively, use the response object directly:
$response = $this->action( ... ); - $this->assertResponseStatus(404); + $response->assertStatus(404);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitRefundPolicyTypeApiTest.php` around lines 136 - 146, In OAuth2SummitRefundPolicyTypeApiTest, the $response variable is assigned from $this->action(...) but never used; either remove the assignment and call $this->action(...) directly before asserting with $this->assertResponseStatus(404), or replace the last assertion with an assertion on the response object (use $response->assertStatus(404)); update the test method where $response is created to use one of these two approaches (reference the $this->action(...)/$response assignment and the $this->assertResponseStatus or $response->assertStatus methods).tests/OAuth2SummitRegistrationInvitationApiControllerTest.php (2)
536-536: Remove unused$responseassignments to satisfy static analysis.The assigned variable is not used in either test method.
Proposed cleanup
- $response = $this->action( + $this->action( "GET", "OAuth2SummitRegistrationInvitationApiController@get", $params, [], [], [], $headers );- $response = $this->action( + $this->action( "DELETE", "OAuth2SummitRegistrationInvitationApiController@delete", $params, [], [], [], $headers );Also applies to: 562-562
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitRegistrationInvitationApiControllerTest.php` at line 536, Remove the unused local variable assignment to $response where the test calls $this->action(...); instead of "$response = $this->action(...);" just invoke "$this->action(...);" in the test methods (the occurrences that currently assign to $response around the calls to action). Update both occurrences (the one at the existing $response assignment and the similar occurrence around line 562) so the return value is not assigned to an unused $response variable, satisfying static analysis.
363-399: Remove unreachable test code aftermarkTestSkipped()calls.
markTestSkipped()throws an exception that immediately halts test method execution. The test bodies at lines 366–399, 405–428, and 434–455 are never executed. Remove the unreachable code, keeping only the skip call, to prevent silent drift and eliminate false confidence from untested logic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitRegistrationInvitationApiControllerTest.php` around lines 363 - 399, Remove the unreachable test body after the markTestSkipped() call in the test method (the block that creates an invitation, calls the controller update, and asserts is_accepted/accepted_date). Keep only the single markTestSkipped(...) line and delete all subsequent code in that test method (references: the markTestSkipped invocation and the use of $this->testInviteWithInvitation(), $this->action(... "OAuth2SummitRegistrationInvitationApiController@update" ...), and the assertions) so the test contains only the skip and no unreachable assertions or setup.tests/OAuth2SummitRSVPTemplateApiTest.php (1)
141-145: Extract the builders out of the publictest*methods.
testAddRSVPTemplate()andtestAddRSVPQuestionValue()are being reused as fixtures directly from other tests. This couples otherwise independent cases and makes one setup failure fan out across multiple tests. Additionally, private helpers likecreateTemplateWithTextBoxQuestion()andcreateTemplateWithDropDownQuestion()also call these public test methods. Extract private builder methods to maintain PHPUnit isolation and keep failure attribution clear.♻️ Refactor sketch
- public function testAddRSVPTemplate(){ + private function createRSVPTemplate(){ ... return $template; } + public function testAddRSVPTemplate(){ + $template = $this->createRSVPTemplate(); + $this->assertNotNull($template); + } - $template = $this->testAddRSVPTemplate(); + $template = $this->createRSVPTemplate();Applies to all 8 instances: lines 141, 175, 213, 246, 299, 465, 507.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/OAuth2SummitRSVPTemplateApiTest.php` around lines 141 - 145, Several tests call public test methods (testAddRSVPTemplate, testAddRSVPQuestionValue) and helpers (createTemplateWithTextBoxQuestion, createTemplateWithDropDownQuestion) as fixtures, coupling tests; extract those setup steps into private builder helpers (e.g. private createRSVPTemplate(), private createRSVPQuestionValue(), private buildTemplateWithTextBoxQuestion(), private buildTemplateWithDropDownQuestion()) and have both the public test methods and other helpers call these private builders instead of calling test* methods directly, updating all references (including createTemplateWithTextBoxQuestion and createTemplateWithDropDownQuestion) to use the new builders so each PHPUnit test remains isolated and failures map to the correct test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/OAuth2SummitProposedScheduleAllowedLocationApiControllerTest.php`:
- Around line 325-328: The comment in testRemoveAllAllowedLocationsFromTrack()
incorrectly says "add two allowed locations" but only one call to
testAddAllowedLocation2Track() is made; either change the comment to "add one
allowed location" or actually add a second allowed location by calling
testAddAllowedLocation2Track() again (or another helper that adds a different
allowed location) before proceeding; update the comment and/or add the second
call in the testRemoveAllAllowedLocationsFromTrack() method to keep intent and
implementation consistent.
In `@tests/OAuth2SummitRefundPolicyTypeApiTest.php`:
- Around line 179-180: The test currently asserts a 201 status for a PUT update;
change the assertion in OAuth2SummitRefundPolicyTypeApiTest (the line with
$this->assertResponseStatus(201) immediately after $content =
$response->getContent()) to $this->assertResponseStatus(200) so the test expects
HTTP 200 for the update instead of 201.
In `@tests/OAuth2SummitRegistrationInvitationApiControllerTest.php`:
- Around line 335-337: The test currently calls another test method
testInviteWithInvitation() from within tests, which creates coupling and side
effects; refactor by extracting the invitation creation/request logic into a
private helper (e.g., createInvitationWithInvitationAudience()) inside the test
class, move all request/response decoding (but not assertions) used by
testInviteWithInvitation() into that helper and have it return the decoded
invitation payload; then replace calls to testInviteWithInvitation() in the
callers with createInvitationWithInvitationAudience() (also update other
occurrences at the mentioned lines) and keep assertions only in the original
testInviteWithInvitation() or move them to dedicated assertion helpers so tests
no longer call each other.
In `@tests/OAuth2SummitRSVPTemplateApiTest.php`:
- Around line 139-170: Update the assertions in testGetRSVPTemplateById (and the
other referenced tests) to verify the actual payload fields instead of only
checking for non-null: assert that $rsvp_template->id equals $template->id,
assert the returned ->label matches the expected/updated label used in the test,
assert dropdown templates include the expected ->class_name, and assert the
returned ->values array contains an entry with the expected created value and
label (and/or count), so the test validates id, label, class_name and
value/label contents rather than just presence.
In `@tests/OAuth2SummitSelectedPresentationListApiTest.php`:
- Line 507: Remove the unused local variable by calling
testAddIndividualSelectionListAndAddSelection() without assigning its return
value: locate the line that assigns $selection_list from
testAddIndividualSelectionListAndAddSelection() and remove the "$selection_list
= " part so the method is invoked for its side effects only (or alternatively
use the returned value if intended), ensuring no unused variable remains.
---
Outside diff comments:
In `@tests/OAuth2SummitRSVPTemplateApiTest.php`:
- Around line 211-239: After performing the DELETE in testDeleteRSVPTemplate,
re-fetch the same resource to ensure it was actually removed: call the GET
action (e.g., use "GET" to
"OAuth2SummitRSVPTemplatesApiController@getRSVPTemplate" with the same
id/template_id headers/params) and assert a 404 (or appropriate not-found)
response; apply the same pattern to the other delete tests referenced (the
delete tests around lines 389-418 and 505-535) so each delete is followed by an
explicit GET/assert-not-found check to prevent false positives.
---
Duplicate comments:
In @.github/workflows/push.yml:
- Around line 73-136: The workflow uses path-like PHPUnit --filter values (e.g.
entries for OAuth2SummitTicketTypesApiTest, OAuth2SummitTicketsApiTest,
OAuth2TrackGroupsApiTest, etc.) which won't match test names; update each list
item so the --filter is a test-name regex (not a file path). For every item
where filter contains "test/" or "tests/", replace the filter value with the
corresponding test class name from the name field (e.g. --filter
OAuth2SummitTicketTypesApiTest) or a proper regex that matches the PHPUnit test
name, and apply this normalization consistently across all entries in the
affected block.
---
Nitpick comments:
In `@tests/OAuth2SummitProposedScheduleAllowedLocationApiControllerTest.php`:
- Around line 312-322: The test assigns the result of the DELETE action to an
unused $response variable in the
OAuth2SummitProposedScheduleAllowedLocationApiController@removeAllowedLocationFromTrack
call; remove the assignment or prefix the variable with an underscore (e.g.,
$_response) to signal intentional non-use so static analysis stops complaining,
leaving the action invocation and subsequent $this->assertResponseStatus(204)
intact in OAuth2SummitProposedScheduleAllowedLocationApiControllerTest.
- Around line 385-395: The test assigns the result of $this->action(...) to an
unused $response variable; remove the unnecessary assignment and just call
$this->action(...) (or, if you intended to inspect the response, use $response
in assertions) for the call to
OAuth2SummitProposedScheduleAllowedLocationApiController@removeTimeFrameFromAllowedLocation
so that the code no longer creates an unused $response variable before
assertResponseStatus(204).
In `@tests/OAuth2SummitProposedScheduleApiTest.php`:
- Around line 151-183: The test method testPublishAll is being unconditionally
skipped which hides coverage; either add a TODO comment in testPublishAll
referencing a tracking ticket/issue ID and why it's skipped, or update the test
setup so testPublishScheduleEvent and its generated presentations do not produce
overlapping timeframes (adjust created presentations' start/end times or
blackout rules) so publishAll (controller method publishAll) can run; ensure the
TODO references testPublishAll and the related test helper
testPublishScheduleEvent so the skip is tracked or remove the skip after fixing
the test data.
In `@tests/OAuth2SummitRefundPolicyTypeApiTest.php`:
- Around line 210-211: Remove the unused $content variable retrieval in the
test: delete the line that assigns $content = $response->getContent(); in the
OAuth2SummitRefundPolicyTypeApiTest test method (the block that follows the
$response handling and before $this->assertResponseStatus(204)); leave only the
assertion $this->assertResponseStatus(204) since a 204 response has no body.
- Around line 225-235: Remove the unused $response variable assignment from the
DELETE test call: instead of assigning the result of $this->action(...) to
$response, call $this->action(...) directly since the test only asserts the
response status; update the test in OAuth2SummitRefundPolicyTypeApiTest.php
where the DELETE call targets "OAuth2SummitRefundPolicyTypeApiController@delete"
to eliminate the unused $response variable.
- Around line 136-146: In OAuth2SummitRefundPolicyTypeApiTest, the $response
variable is assigned from $this->action(...) but never used; either remove the
assignment and call $this->action(...) directly before asserting with
$this->assertResponseStatus(404), or replace the last assertion with an
assertion on the response object (use $response->assertStatus(404)); update the
test method where $response is created to use one of these two approaches
(reference the $this->action(...)/$response assignment and the
$this->assertResponseStatus or $response->assertStatus methods).
In `@tests/OAuth2SummitRegistrationCompaniesApiTest.php`:
- Around line 98-99: The test assigns $content = $response->getContent() but
never uses it; either remove that unused assignment or add an assertion that
validates the response body (for example assertNotEmpty($content) or an
appropriate JSON/assertion) after the existing $this->assertResponseStatus(201)
in OAuth2SummitRegistrationCompaniesApiTest (locate the $content =
$response->getContent() line and update it accordingly).
- Around line 135-137: The test uses Storage::fake('swift') and Queue::fake()
but never verifies queued jobs; after the existing assertResponseStatus(200) in
OAuth2SummitRegistrationCompaniesApiTest you should add a queue assertion: if
the import endpoint is supposed to dispatch a job, call
Queue::assertPushed(YourExpectedJob::class) (use the actual job class name
dispatched by the import flow), otherwise call Queue::assertNothingPushed() to
assert no job was queued; place the assertion immediately after the response
assertion so it validates dispatch behavior for the import endpoint.
- Around line 129-130: Remove the unused assignment to $content in
OAuth2SummitRegistrationCompaniesApiTest.php: the line calling
$response->getContent() and assigning to $content should be deleted (or replaced
with an assertion that actually checks the content if intended); locate the code
around the test where $content = $response->getContent(); and keep the existing
$this->assertResponseStatus(204) call, removing the unused $content variable to
eliminate the dead assignment.
In `@tests/OAuth2SummitRegistrationInvitationApiControllerTest.php`:
- Line 536: Remove the unused local variable assignment to $response where the
test calls $this->action(...); instead of "$response = $this->action(...);" just
invoke "$this->action(...);" in the test methods (the occurrences that currently
assign to $response around the calls to action). Update both occurrences (the
one at the existing $response assignment and the similar occurrence around line
562) so the return value is not assigned to an unused $response variable,
satisfying static analysis.
- Around line 363-399: Remove the unreachable test body after the
markTestSkipped() call in the test method (the block that creates an invitation,
calls the controller update, and asserts is_accepted/accepted_date). Keep only
the single markTestSkipped(...) line and delete all subsequent code in that test
method (references: the markTestSkipped invocation and the use of
$this->testInviteWithInvitation(), $this->action(...
"OAuth2SummitRegistrationInvitationApiController@update" ...), and the
assertions) so the test contains only the skip and no unreachable assertions or
setup.
In `@tests/OAuth2SummitRSVPTemplateApiTest.php`:
- Around line 141-145: Several tests call public test methods
(testAddRSVPTemplate, testAddRSVPQuestionValue) and helpers
(createTemplateWithTextBoxQuestion, createTemplateWithDropDownQuestion) as
fixtures, coupling tests; extract those setup steps into private builder helpers
(e.g. private createRSVPTemplate(), private createRSVPQuestionValue(), private
buildTemplateWithTextBoxQuestion(), private buildTemplateWithDropDownQuestion())
and have both the public test methods and other helpers call these private
builders instead of calling test* methods directly, updating all references
(including createTemplateWithTextBoxQuestion and
createTemplateWithDropDownQuestion) to use the new builders so each PHPUnit test
remains isolated and failures map to the correct test.
In `@tests/OAuth2SummitScheduleSettingsApiTest.php`:
- Around line 709-717: Remove the unused $response assignments where the test
only performs a status assertion: replace the assignments like $response =
$this->action("GET", "OAuth2SummitScheduleSettingsApiController@get", ...) with
a direct call to $this->action(...) (no assignment) in both locations (the GET
to OAuth2SummitScheduleSettingsApiController@get and the other similar call
around lines 747-755), leaving the subsequent assertResponseStatus(...) checks
unchanged.
- Around line 722-732: Update the skipped tests to reference concrete tracker
IDs so they are discoverable: replace the current markTestSkipped messages in
testGetMetadata, testAddFilterToConfig, and testUpdateFilterById with messages
that include the corresponding issue/bug/SONAR/TRELLO/JIRA IDs (e.g. "SKIP:
issue XYZ-123 - Controller getMetadata() is unimplemented"), and add a TODO
annotation above each test linking the same ID; if no issue exists, create one
for each (Controller: getMetadata, Service:
SummitScheduleSettingsService::addFilter, Service:
SummitScheduleSettingsService::updateFilter) and use that new ID in the skip
message so CI and reviewers can track remediation.
In `@tests/OAuth2SummitSelectionPlanExtraQuestionTypeApiTest.php`:
- Around line 94-95: Replace the generic boolean assertions in the
OAuth2SummitSelectionPlanExtraQuestionTypeApiTest test with PHPUnit's idiomatic
assertions: use assertNotNull($page) instead of assertTrue(!is_null($page)) and
use assertGreaterThanOrEqual(20, $page->total) (or assertGreaterThan(19,
$page->total)) instead of assertTrue($page->total >= 20) so failures show
clearer messages and expected vs actual values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c47b481b-332b-4635-85cc-52ec3e6e3d9f
📒 Files selected for processing (11)
.github/workflows/push.ymlapp/ModelSerializers/Summit/Registration/SummitRegistrationInvitationCSVSerializer.phptests/OAuth2SummitProposedScheduleAllowedLocationApiControllerTest.phptests/OAuth2SummitProposedScheduleApiTest.phptests/OAuth2SummitRSVPTemplateApiTest.phptests/OAuth2SummitRefundPolicyTypeApiTest.phptests/OAuth2SummitRegistrationCompaniesApiTest.phptests/OAuth2SummitRegistrationInvitationApiControllerTest.phptests/OAuth2SummitScheduleSettingsApiTest.phptests/OAuth2SummitSelectedPresentationListApiTest.phptests/OAuth2SummitSelectionPlanExtraQuestionTypeApiTest.php
| public function testRemoveAllAllowedLocationsFromTrack() | ||
| { | ||
| // add two allowed locations | ||
| $this->testAddAllowedLocation2Track(); |
There was a problem hiding this comment.
Misleading comment: claims to add two locations but only adds one.
The comment on line 327 states "add two allowed locations" but only one call to testAddAllowedLocation2Track() is made. Either update the comment or add a second location if the intent is to test removal of multiple locations.
✏️ Suggested fix
public function testRemoveAllAllowedLocationsFromTrack()
{
- // add two allowed locations
- $this->testAddAllowedLocation2Track();
+ // add an allowed location
+ $this->testAddAllowedLocation2Track();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public function testRemoveAllAllowedLocationsFromTrack() | |
| { | |
| // add two allowed locations | |
| $this->testAddAllowedLocation2Track(); | |
| public function testRemoveAllAllowedLocationsFromTrack() | |
| { | |
| // add an allowed location | |
| $this->testAddAllowedLocation2Track(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SummitProposedScheduleAllowedLocationApiControllerTest.php`
around lines 325 - 328, The comment in testRemoveAllAllowedLocationsFromTrack()
incorrectly says "add two allowed locations" but only one call to
testAddAllowedLocation2Track() is made; either change the comment to "add one
allowed location" or actually add a second allowed location by calling
testAddAllowedLocation2Track() again (or another helper that adds a different
allowed location) before proceeding; update the comment and/or add the second
call in the testRemoveAllAllowedLocationsFromTrack() method to keep intent and
implementation consistent.
| $content = $response->getContent(); | ||
| $this->assertResponseStatus(201); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check what status code the update action returns in the controller
# Search for the update method in the refund policy controller
ast-grep --pattern $'function update($_) {
$$$
}'
# Also check for any return statements with status codes
rg -n -A10 'function update' --glob '*RefundPolicyTypeApiController*'Repository: OpenStackweb/summit-api
Length of output: 710
🏁 Script executed:
#!/bin/bash
# Find the parent class of OAuth2SummitRefundPolicyTypeApiController
rg -n 'class OAuth2SummitRefundPolicyTypeApiController' -A 2 --glob '*RefundPolicyTypeApiController*'
# Check for response handling that might set 201
rg -n 'status\(201\)|response.*201' --glob '*RefundPolicyTypeApiController*'
# Look at the test file to understand the full context
head -190 tests/OAuth2SummitRefundPolicyTypeApiTest.php | tail -50Repository: OpenStackweb/summit-api
Length of output: 1784
🏁 Script executed:
#!/bin/bash
# Find OAuth2ProtectedController
find . -name '*OAuth2ProtectedController*' -type f
# Look at the parent controller to see response handling
rg -n 'response.*201|status.*201' --glob '*ProtectedController*' -A 3 -B 3
# Check what testAddPolicy returns to understand pattern
rg -n 'testAddPolicy' tests/OAuth2SummitRefundPolicyTypeApiTest.php -A 15Repository: OpenStackweb/summit-api
Length of output: 2866
🏁 Script executed:
#!/bin/bash
# Get the complete testAddPolicy method with status check
sed -n '31,80p' tests/OAuth2SummitRefundPolicyTypeApiTest.php
# Also check OAuth2ProtectedController for any response status handling
head -100 app/Http/Controllers/OAuth2ProtectedController.php
# Look for patterns of POST/PUT status codes in other tests
rg -n 'assertResponseStatus\(20[01]\)' tests/OAuth2SummitRefundPolicyTypeApiTest.phpRepository: OpenStackweb/summit-api
Length of output: 3387
🏁 Script executed:
#!/bin/bash
# Check what methods use 200 status code assertions
sed -n '80,125p' tests/OAuth2SummitRefundPolicyTypeApiTest.phpRepository: OpenStackweb/summit-api
Length of output: 1286
Change assertion on line 180 from assertResponseStatus(201) to assertResponseStatus(200).
HTTP 201 (Created) is for new resource creation via POST. PUT requests for updates should return HTTP 200 (OK). The test incorrectly asserts 201 for the PUT update; this should be 200 to match REST conventions where:
- POST creates → 201
- PUT updates → 200
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SummitRefundPolicyTypeApiTest.php` around lines 179 - 180, The
test currently asserts a 201 status for a PUT update; change the assertion in
OAuth2SummitRefundPolicyTypeApiTest (the line with
$this->assertResponseStatus(201) immediately after $content =
$response->getContent()) to $this->assertResponseStatus(200) so the test expects
HTTP 200 for the update instead of 201.
| // first create an invitation so there's data to export | ||
| $this->testInviteWithInvitation(); | ||
|
|
There was a problem hiding this comment.
Avoid calling test methods from other tests.
These calls couple tests and inherit assertions/side effects from another test case, which makes failures noisy and order-sensitive. Extract a shared helper/factory method instead.
Proposed refactor
- $this->testInviteWithInvitation();
+ $this->createInvitationWithInvitationAudience();private function createInvitationWithInvitationAudience()
{
// move the invitation creation request logic here
// return decoded invitation payload
}Also applies to: 366-367, 460-461, 495-496, 550-551, 590-591
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SummitRegistrationInvitationApiControllerTest.php` around lines
335 - 337, The test currently calls another test method
testInviteWithInvitation() from within tests, which creates coupling and side
effects; refactor by extracting the invitation creation/request logic into a
private helper (e.g., createInvitationWithInvitationAudience()) inside the test
class, move all request/response decoding (but not assertions) used by
testInviteWithInvitation() into that helper and have it return the decoded
invitation payload; then replace calls to testInviteWithInvitation() in the
callers with createInvitationWithInvitationAudience() (also update other
occurrences at the mentioned lines) and keep assertions only in the original
testInviteWithInvitation() or move them to dedicated assertion helpers so tests
no longer call each other.
| public function testGetRSVPTemplateById(){ | ||
|
|
||
| $template = $this->testAddRSVPTemplate($summit_id); | ||
| $template = $this->testAddRSVPTemplate(); | ||
|
|
||
| $params = [ | ||
| 'id' => $summit_id, | ||
| 'id' => $this->summitId(), | ||
| 'template_id' => $template->id, | ||
| ]; | ||
|
|
||
| $headers = | ||
| [ | ||
| "HTTP_Authorization" => " Bearer " . $this->access_token, | ||
| "CONTENT_TYPE" => "application/json" | ||
| ]; | ||
|
|
||
| $response = $this->action | ||
| ( | ||
| "GET", | ||
| "OAuth2SummitRSVPTemplatesApiController@getRSVPTemplate", | ||
| $params, | ||
| [], | ||
| [], | ||
| [], | ||
| $headers | ||
| ); | ||
|
|
||
| $content = $response->getContent(); | ||
| $this->assertResponseStatus(200); | ||
|
|
||
| $rsvp_template = json_decode($content); | ||
| $this->assertTrue(!is_null($rsvp_template)); | ||
| return $rsvp_template; |
There was a problem hiding this comment.
Tighten the object assertions.
These cases mostly stop at not null, so they still pass if the API returns the wrong template/question/value. Please assert the expected id, updated label, dropdown class_name, and created value/label so the test proves the payload, not just the transport.
Also applies to: 341-345, 347-387, 420-460
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SummitRSVPTemplateApiTest.php` around lines 139 - 170, Update the
assertions in testGetRSVPTemplateById (and the other referenced tests) to verify
the actual payload fields instead of only checking for non-null: assert that
$rsvp_template->id equals $template->id, assert the returned ->label matches the
expected/updated label used in the test, assert dropdown templates include the
expected ->class_name, and assert the returned ->values array contains an entry
with the expected created value and label (and/or count), so the test validates
id, label, class_name and value/label contents rather than just presence.
| */ | ||
| public function testAddAndRemovePresentationFromIndividualList(){ | ||
|
|
||
| $selection_list = $this->testAddIndividualSelectionListAndAddSelection(); |
There was a problem hiding this comment.
Remove unused local $selection_list in Line 507.
It is assigned but never read, which adds noise and matches the static-analysis warning.
Proposed cleanup
- $selection_list = $this->testAddIndividualSelectionListAndAddSelection();
+ $this->testAddIndividualSelectionListAndAddSelection();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| $selection_list = $this->testAddIndividualSelectionListAndAddSelection(); | |
| $this->testAddIndividualSelectionListAndAddSelection(); |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 507-507: Avoid unused local variables such as '$selection_list'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/OAuth2SummitSelectedPresentationListApiTest.php` at line 507, Remove
the unused local variable by calling
testAddIndividualSelectionListAndAddSelection() without assigning its return
value: locate the line that assigns $selection_list from
testAddIndividualSelectionListAndAddSelection() and remove the "$selection_list
= " part so the method is invoked for its side effects only (or alternatively
use the returned value if intended), ensuring no unused variable remains.
There was a problem hiding this comment.
Pull request overview
This PR modernizes and stabilizes the PHP test suite (mostly OAuth2 protected API integration tests) by updating PHPUnit coverage annotations, improving the test access-token stub, and removing hard-coded IDs in favor of seeded test data.
Changes:
- Migrate PHPUnit
@coversdocblocks to#[CoversClass]attributes in RSVP service tests. - Extend
tests/ProtectedApiTestCase.phpaccess-token stub with additional user identity claims and an elections read scope. - Refactor multiple OAuth2 API tests to use seeded summit/member data (and add coverage for sub-tracks and get-by-id endpoints), reducing reliance on fixed IDs.
Reviewed changes
Copilot reviewed 65 out of 65 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/SummitRSVPServiceTest.php | Replace @covers docblock with #[CoversClass]. |
| tests/SummitRSVPInvitationServiceTest.php | Replace @covers docblock with #[CoversClass]. |
| tests/ProtectedApiTestCase.php | Add user email/first/last name to the access-token stub + include an elections scope; populate these in setUp(). |
| tests/OAuth2TrackTagGroupsApiTest.php | Align setup with shared member fixture (self::$defaultMember) and rely on base test data seeding. |
| tests/OAuth2TracksApiTest.php | Remove hard-coded filters/IDs, add missing-file icon test, ensure delete track uses a newly created track, add sub-track tests. |
| tests/OAuth2TrackQuestionsTemplateTest.php | Add summit seeding and remove hard-coded IDs; add get-by-id tests for templates/values. |
| tests/OAuth2TrackGroupsApiTest.php | Remove hard-coded summit IDs and align ordering/filtering with repository mappings; add CSV test; adjust missing-track assertion to 404. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| protected function setUp(): void | ||
| { | ||
| $this->setCurrentGroup(IGroup::TrackChairs); | ||
| parent::setUp(); | ||
| self::$defaultMember = self::$member; | ||
| self::insertSummitTestData(); |
| /** | ||
| * @param int $summit_id | ||
| */ | ||
| public function testGetTrackGroupsPrivate($summit_id = 23) | ||
| public function testGetTrackGroupsPrivate() | ||
| { |
| public function testAssociateTrack2TrackGroup412(){ | ||
|
|
||
| $track_group = $this->testAddTrackGroup($summit_id); | ||
| $track_group = $this->testAddTrackGroup(); |
ref: https://app.clickup.com/t/86b8t9mdd?utm_source=email-notifications&utm_type=1&utm_field=start_date
Summary by CodeRabbit
New Features
Bug Fixes
Improvements