test(integration): add WebApplicationFactory HTTP tests (#421)#464
test(integration): add WebApplicationFactory HTTP tests (#421)#464nanotaboada merged 1 commit intomasterfrom
WebApplicationFactory HTTP tests (#421)#464Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 12 minutes and 45 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
WalkthroughThis PR implements HTTP-layer integration tests for ASP.NET Core player endpoints using Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nanotaboada
changed the title
WebApplicationFactory HTTP tests (#421)
nanotaboada
force-pushed
the
feat/web-application-integration-tests
branch
from
a83e8a4 to
2d9dc55
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
test/Dotnet.Samples.AspNetCore.WebApi.Tests/Integration/PlayerWebApplicationTests.cs (3)
95-96: Avoid pinning this test to today's seed count.
HaveCount(26)will fail on any legitimate seed-data change even ifGET /playersstill returns the full dataset. Derive the expected count from the seeded test database or a shared fixture instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Dotnet.Samples.AspNetCore.WebApi.Tests/Integration/PlayerWebApplicationTests.cs` around lines 95 - 96, The test in PlayerWebApplicationTests pins the expected count to 26 (players.Should().HaveCount(26)); instead derive the expected value from the seeded test data or shared fixture instead of hardcoding. Update the assertion to compare players.Count to the seed source exported by your test fixture or test DbContext (e.g., use SeededPlayers.Count from the test fixture, or query the test IServiceProvider/DbContext used by PlayerWebApplicationTests to get the seeded players count) so the test asserts players.Should().HaveCount(expectedCount) where expectedCount is obtained at runtime from the seed.
317-319: Rename this test to keep the suite's naming pattern consistent.
Get_Health_Returns200Okis missing the condition segment used everywhere else in this class. Something likeGet_Health_Healthy_Returns200Okwould match the project convention better.As per coding guidelines, "Controller tests:
{HttpMethod}_{Resource}_{Condition}_Returns{Outcome}".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Dotnet.Samples.AspNetCore.WebApi.Tests/Integration/PlayerWebApplicationTests.cs` around lines 317 - 319, The test method Get_Health_Returns200Ok in PlayerWebApplicationTests should be renamed to include the condition segment to match the project's controller test naming convention; change the method identifier from Get_Health_Returns200Ok to Get_Health_Healthy_Returns200Ok (and update any references/usages) so it follows {HttpMethod}_{Resource}_{Condition}_Returns{Outcome}; keep the same attributes ([Fact], [Trait]) and body, only update the method name.
129-132: Strengthen the RFC 7807 assertions.Checking only
StatusandTitlestill lets a non-Problem Details 404 body slip through. Since this suite is meant to pin the HTTP contract, add at least theapplication/problem+jsoncontent type and one stable RFC 7807 field per error category; for validation cases, assert the validation payload rather than just the 400.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/Dotnet.Samples.AspNetCore.WebApi.Tests/Integration/PlayerWebApplicationTests.cs` around lines 129 - 132, The test only checks Status and Title and can miss non-RFC7807 responses; update the assertions to also verify the response.Content.Headers.ContentType.MediaType equals "application/problem+json" and assert a stable RFC7807 field on the deserialized ProblemDetails (for this 404 case check problem.Type or problem.Instance is set to the expected value or at least not null), and for any 400/validation tests ensure you deserialize to ValidationProblemDetails and assert its Errors dictionary contains the expected keys/values (use response, problem, ProblemDetails, ValidationProblemDetails references to locate where to change).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@adr/0013-testing-strategy.md`:
- Around line 35-45: The markdown lint violation MD022 is caused because the
headings "### Positive", "### Negative", and "### Neutral" are not followed by
blank lines; update the ADR content so each of those subsection headings is
immediately followed by a single blank line before their list or paragraph
content (i.e., insert an empty line after "### Positive", after "### Negative",
and after "### Neutral") to satisfy the lint rule.
In
`@test/Dotnet.Samples.AspNetCore.WebApi.Tests/Integration/PlayerWebApplicationTests.cs`:
- Around line 60-67: The test suite currently registers TestAuthHandler as the
default scheme in the shared factory via AddAuthentication/AddScheme (using
TestAuthHandler.SchemeName), which authenticates every request; instead remove
that registration from the shared factory in PlayerWebApplicationTests and
create a dedicated factory/configuration used only by the authorized test(s)
(the GET /players/{id} test) that adds
AddAuthentication(...).AddScheme<AuthenticationSchemeOptions,
TestAuthHandler>(TestAuthHandler.SchemeName, _ => { }); update the single test
that needs [Authorize] to use this dedicated factory so the other 12 tests
continue running unauthenticated and will fail if an endpoint accidentally
becomes protected.
---
Nitpick comments:
In
`@test/Dotnet.Samples.AspNetCore.WebApi.Tests/Integration/PlayerWebApplicationTests.cs`:
- Around line 95-96: The test in PlayerWebApplicationTests pins the expected
count to 26 (players.Should().HaveCount(26)); instead derive the expected value
from the seeded test data or shared fixture instead of hardcoding. Update the
assertion to compare players.Count to the seed source exported by your test
fixture or test DbContext (e.g., use SeededPlayers.Count from the test fixture,
or query the test IServiceProvider/DbContext used by PlayerWebApplicationTests
to get the seeded players count) so the test asserts
players.Should().HaveCount(expectedCount) where expectedCount is obtained at
runtime from the seed.
- Around line 317-319: The test method Get_Health_Returns200Ok in
PlayerWebApplicationTests should be renamed to include the condition segment to
match the project's controller test naming convention; change the method
identifier from Get_Health_Returns200Ok to Get_Health_Healthy_Returns200Ok (and
update any references/usages) so it follows
{HttpMethod}_{Resource}_{Condition}_Returns{Outcome}; keep the same attributes
([Fact], [Trait]) and body, only update the method name.
- Around line 129-132: The test only checks Status and Title and can miss
non-RFC7807 responses; update the assertions to also verify the
response.Content.Headers.ContentType.MediaType equals "application/problem+json"
and assert a stable RFC7807 field on the deserialized ProblemDetails (for this
404 case check problem.Type or problem.Instance is set to the expected value or
at least not null), and for any 400/validation tests ensure you deserialize to
ValidationProblemDetails and assert its Errors dictionary contains the expected
keys/values (use response, problem, ProblemDetails, ValidationProblemDetails
references to locate where to change).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: cf34f8e6-fe20-41e8-9e64-7c5571d89067
⛔ Files ignored due to path filters (1)
test/Dotnet.Samples.AspNetCore.WebApi.Tests/packages.lock.jsonis excluded by!**/packages.lock.json
📒 Files selected for processing (7)
CHANGELOG.mdadr/0013-testing-strategy.mdadr/README.mdsrc/Dotnet.Samples.AspNetCore.WebApi/Program.cstest/Dotnet.Samples.AspNetCore.WebApi.Tests/Dotnet.Samples.AspNetCore.WebApi.Tests.csprojtest/Dotnet.Samples.AspNetCore.WebApi.Tests/Integration/PlayerWebApplicationTests.cstest/Dotnet.Samples.AspNetCore.WebApi.Tests/Utilities/TestAuthHandler.cs
nanotaboada
force-pushed
the
feat/web-application-integration-tests
branch
2 times, most recently
from
8f97783 to
872169d
Compare
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
nanotaboada
force-pushed
the
feat/web-application-integration-tests
branch
from
872169d to
eedf3f5
Compare
|
|
@coderabbitai addressing the three nitpick comments from the review:
Strengthening RFC 7807 assertions — not actioned. |
|
Tip For best results, initiate chat on the files or code changes.
✏️ Learnings added
🧠 Learnings usedIf you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
1 participant
Summary
PlayerWebApplicationTests— 14 HTTP-layer integration tests covering all player endpoints and/healthviaWebApplicationFactory<Program>backed by in-memory SQLiteTestAuthHandlerutility to bypass[Authorize]onGET /players/{id:Guid}in testsProgramto the test project viapublic partial class Program {}inProgram.csMicrosoft.AspNetCore.Mvc.Testingto the test projectTest plan
dotnet build --configuration Releasepassesdotnet test --settings .runsettings— 64 tests, 0 failuresdotnet csharpier --check .passesCloses #421
🤖 Generated with Claude Code
Summary by CodeRabbit
Tests
Documentation
Chores