Limit Period for Subreddits #174
Conversation
WalkthroughA single test method is refactored from a parameterless fact-based test to a parameterized theory test accepting multiple URL inputs. Skip logic is enhanced to conditionally assess GitHub Actions environment and URL context, with the test now using provided URLs instead of hardcoded values. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Elzik.Breef.Api.Tests.Functional/Breefs/BreefTestsBase.cs (1)
20-20: Consider reverting the setter if the mutation is removed.If you adopt the local variable approach suggested for lines 47-57, the private setter added to
SkipTestsReasonbecomes unnecessary, and the property can remain read-only.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/Elzik.Breef.Api.Tests.Functional/Breefs/BreefTestsBase.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: build-ubuntu
- GitHub Check: Analyze (csharp)
- GitHub Check: build-ubuntu
🔇 Additional comments (2)
tests/Elzik.Breef.Api.Tests.Functional/Breefs/BreefTestsBase.cs (2)
41-45: Parameterization looks good.The conversion from
[SkippableFact]to[SkippableTheory]with multiple URL test cases is appropriate for testing both regular URLs and Reddit-specific behavior.
60-60: LGTM!The URL parameter is correctly used in the test payload, enabling the parameterized test to work with different URLs.
| var attemptingRedditTestInGithubWorklow = | ||
| url.StartsWith("https://www.reddit.com") && | ||
| Environment.GetEnvironmentVariable("GITHUB_ACTIONS") == "true"; | ||
| var skipTest = SkipTestsIf || attemptingRedditTestInGithubWorklow; | ||
| if(attemptingRedditTestInGithubWorklow) | ||
| { | ||
| SkipTestsReason = "Skipped because requests to reddit.com from GitHub workflows are " + | ||
| "always blocked meaning this test case always fails. This must be run locally instead."; | ||
| } | ||
|
|
||
| Skip.If(skipTest, SkipTestsReason); |
There was a problem hiding this comment.
Race condition: mutating shared property in parameterized test.
Modifying SkipTestsReason (line 53) within a theory test creates a race condition because xUnit runs theory tests in parallel by default. Multiple test instances can overwrite each other's skip reasons, leading to incorrect or misleading skip messages.
Recommended solution: Use a local variable for the skip reason instead:
[SkippableTheory]
[InlineData("http://example.com")]
[InlineData("https://www.reddit.com/r/dotnet/")]
[InlineData("https://www.reddit.com/r/selfhosted/comments/1ojndg6/advice_should_i_buy_a_new_router_or_build_one/")]
public async Task EndToEndHappyPath(string url)
{
var attemptingRedditTestInGithubWorklow =
url.StartsWith("https://www.reddit.com") &&
Environment.GetEnvironmentVariable("GITHUB_ACTIONS") == "true";
var skipTest = SkipTestsIf || attemptingRedditTestInGithubWorklow;
+ var skipReason = SkipTestsReason;
if(attemptingRedditTestInGithubWorklow)
{
- SkipTestsReason = "Skipped because requests to reddit.com from GitHub workflows are " +
+ skipReason = "Skipped because requests to reddit.com from GitHub workflows are " +
"always blocked meaning this test case always fails. This must be run locally instead.";
}
- Skip.If(skipTest, SkipTestsReason);
+ Skip.If(skipTest, skipReason);Minor: Use case-insensitive URL comparison.
The url.StartsWith("https://www.reddit.com") check (line 48) is case-sensitive. For robustness, use case-insensitive comparison for the URL scheme and host:
- var attemptingRedditTestInGithubWorklow =
- url.StartsWith("https://www.reddit.com") &&
- Environment.GetEnvironmentVariable("GITHUB_ACTIONS") == "true";
+ var attemptingRedditTestInGithubWorklow =
+ url.StartsWith("https://www.reddit.com", StringComparison.OrdinalIgnoreCase) &&
+ Environment.GetEnvironmentVariable("GITHUB_ACTIONS") == "true";📝 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.
| var attemptingRedditTestInGithubWorklow = | |
| url.StartsWith("https://www.reddit.com") && | |
| Environment.GetEnvironmentVariable("GITHUB_ACTIONS") == "true"; | |
| var skipTest = SkipTestsIf || attemptingRedditTestInGithubWorklow; | |
| if(attemptingRedditTestInGithubWorklow) | |
| { | |
| SkipTestsReason = "Skipped because requests to reddit.com from GitHub workflows are " + | |
| "always blocked meaning this test case always fails. This must be run locally instead."; | |
| } | |
| Skip.If(skipTest, SkipTestsReason); | |
| var attemptingRedditTestInGithubWorklow = | |
| url.StartsWith("https://www.reddit.com") && | |
| Environment.GetEnvironmentVariable("GITHUB_ACTIONS") == "true"; | |
| var skipTest = SkipTestsIf || attemptingRedditTestInGithubWorklow; | |
| var skipReason = SkipTestsReason; | |
| if(attemptingRedditTestInGithubWorklow) | |
| { | |
| skipReason = "Skipped because requests to reddit.com from GitHub workflows are " + | |
| "always blocked meaning this test case always fails. This must be run locally instead."; | |
| } | |
| Skip.If(skipTest, skipReason); |
| var attemptingRedditTestInGithubWorklow = | |
| url.StartsWith("https://www.reddit.com") && | |
| Environment.GetEnvironmentVariable("GITHUB_ACTIONS") == "true"; | |
| var skipTest = SkipTestsIf || attemptingRedditTestInGithubWorklow; | |
| if(attemptingRedditTestInGithubWorklow) | |
| { | |
| SkipTestsReason = "Skipped because requests to reddit.com from GitHub workflows are " + | |
| "always blocked meaning this test case always fails. This must be run locally instead."; | |
| } | |
| Skip.If(skipTest, SkipTestsReason); | |
| var attemptingRedditTestInGithubWorklow = | |
| url.StartsWith("https://www.reddit.com", StringComparison.OrdinalIgnoreCase) && | |
| Environment.GetEnvironmentVariable("GITHUB_ACTIONS") == "true"; | |
| var skipTest = SkipTestsIf || attemptingRedditTestInGithubWorklow; | |
| if(attemptingRedditTestInGithubWorklow) | |
| { | |
| SkipTestsReason = "Skipped because requests to reddit.com from GitHub workflows are " + | |
| "always blocked meaning this test case always fails. This must be run locally instead."; | |
| } | |
| Skip.If(skipTest, SkipTestsReason); |
🤖 Prompt for AI Agents
In tests/Elzik.Breef.Api.Tests.Functional/Breefs/BreefTestsBase.cs around lines
47 to 57, avoid mutating the shared property SkipTestsReason inside the
parameterized/theory test to prevent race conditions: create a local variable
(e.g., localSkipReason) and set that when computing whether to skip, then pass
that local variable into Skip.If; also make the reddit URL check
case-insensitive (e.g., use StartsWith with an OrdinalIgnoreCase comparison or
parse the URL and compare the scheme/host case-insensitively) so the detection
is robust.
|
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.