Skip to content

Conversation

ngtduc693
Copy link
Contributor

Related issue

#543 Unstable behavior in PredictRating() unit test due to constructor field shadowing

Affected module

  • CollaborativeFilteringTests.cs

Root Cause

The instability was not caused by the algorithm itself, but by the test fixture design:

  • The test then fails intermittently, even though the same code passes when executed in isolation
  • When the entire test suite runs in parallel, other tests’ [SetUp] methods overwrite these fields while this test is still executing.
  • The mock that should return a fixed similarity (.Returns(0.8)) is replaced mid-run by a fresh, unconfigured instance, causing:
similarity = 0
PredictRating() → total similarity ≈ 0 → result = 0

Fix Summary

Use the [NonParallelizable] flag

Unit Test Passed Evidence

image
  • I have performed a self-review of my code
  • My code follows the style guidelines of this project
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Comments in areas I changed are up to date
  • I have added comments to hard-to-understand areas of my code
  • I have made corresponding changes to the README.md

Copy link

codecov bot commented Oct 5, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 96.67%. Comparing base (7467fe4) to head (7b4fc36).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #544      +/-   ##
==========================================
- Coverage   96.67%   96.67%   -0.01%     
==========================================
  Files         279      279              
  Lines       11015    11013       -2     
  Branches     1568     1568              
==========================================
- Hits        10649    10647       -2     
  Misses        232      232              
  Partials      134      134              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@ngtduc693 ngtduc693 marked this pull request as ready for review October 5, 2025 16:36
@ngtduc693 ngtduc693 requested a review from siriak as a code owner October 5, 2025 16:36
@siriak
Copy link
Member

siriak commented Oct 5, 2025

Please resolve conflicts

@ngtduc693
Copy link
Contributor Author

ngtduc693 commented Oct 6, 2025

Please resolve conflicts

I'm done, thanks @siriak

@siriak siriak requested a review from Copilot October 6, 2025 06:52
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR fixes an intermittent test failure in the PredictRating() unit test caused by parallel test execution and shared mock instances. The root cause was that concurrent tests were overwriting shared mock objects, leading to unpredictable test results when the test suite ran in parallel.

  • Modernized the CollaborativeFiltering class constructor to use primary constructor syntax
  • Added [NonParallelizable] attribute to the problematic test method to prevent race conditions
  • Removed redundant test methods to simplify the test suite

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
CollaborativeFiltering.cs Updated to use C# primary constructor syntax for cleaner code
CollaborativeFilteringTests.cs Added NonParallelizable attribute and removed duplicate test methods

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link
Member

@siriak siriak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks!

@siriak siriak merged commit a8af880 into TheAlgorithms:master Oct 6, 2025
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants