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
📝 WalkthroughWalkthroughWide-ranging test-suite overhaul: parameterized tests replaced with fixture-driven setup, many new/renamed test cases and lifecycle hooks added; minor production edits include a trait addition, a method type hint, an added 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
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![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.
…, OrderExtraQuestion, PresentationAction, PresentationActionType tests chore(ci): add OAuth2SummitMediaFileTypeApiControllerTest, OAuth2SummitMediaUploadTypeApiControllerTest, OAuth2SummitMetricsApiControllerTest, OAuth2SummitNotificationsApiControllerTest, OAuth2SummitOrderExtraQuestionTypeApiTest, OAuth2SummitOrdersApiTest, OAuth2SummitPresentationActionApiTest, OAuth2SummitPresentationActionTypeApiTest
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
1 participant
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