Skip to content

Conversation

@kasya
Copy link
Collaborator

@kasya kasya commented Oct 29, 2025

Fix SonarQube S1244: Use math.isclose() instead of direct floating point equality in CommitteeNode timestamp test.

Checklist

  • I've read and followed the contributing guidelines.
  • I've run make check-test locally; all checks and tests passed.

@kasya kasya requested a review from arkid15r October 29, 2025 02:30
@kasya kasya added the backend label Oct 29, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 29, 2025

Summary by CodeRabbit

Tests

  • Strengthened floating-point comparison accuracy across multiple test suites to ensure more precise validation of numeric values, timestamps, and calculated results.
  • Enhanced test assertion patterns for improved reliability in validating calculations and time-based comparisons.
  • Updated validation patterns for exact string comparison verification in specific test scenarios.

Walkthrough

The pull request replaces floating-point assertion methods across multiple test files. Several tests switch from pytest.approx() to math.isclose() for comparing floating-point values, while one test changes to exact string comparison. A minor formatting adjustment is also included.

Changes

Cohort / File(s) Summary
Floating-point assertion refactoring (pytest.approx → math.isclose)
backend/tests/apps/owasp/api/internal/nodes/committee_test.py, backend/tests/apps/owasp/api/internal/nodes/common_test.py, backend/tests/apps/github/api/internal/nodes/milestone_test.py, backend/tests/apps/github/api/internal/nodes/user_test.py
Added import math and replaced pytest.approx() assertions with math.isclose() for floating-point comparisons across multiple test cases.
String assertion refactoring
backend/tests/apps/slack/admin/member_test.py
Changed from pytest.approx() to exact string assertion for success message comparison in user assignment test.
Formatting cleanup
backend/tests/apps/owasp/management/commands/owasp_create_project_metadata_file_test.py
Added blank line after assertion for improved readability.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

  • Homogeneous changes across files following consistent pattern
  • No complex logic modifications, only assertion method replacements
  • Changes are straightforward and maintain existing test semantics

Possibly related PRs

Suggested reviewers

  • arkid15r

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 47.83% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The title "Fixed an issue with equality checks on floating point values" is partially related to the changeset. It accurately identifies the problem domain being addressed (floating-point equality comparisons in tests) and reflects the general nature of the changes across multiple test files. However, the title is somewhat vague and doesn't specify the actual solution implemented (replacing pytest.approx() with math.isclose() in most cases). While the title doesn't capture every detail, it sufficiently conveys that the changeset addresses issues with floating-point comparisons.
Description Check ✅ Passed The PR description clearly relates to the changeset by identifying the specific issue being addressed (SonarQube rule S1244) and describing the solution (using math.isclose() instead of direct floating-point equality). While the description specifically mentions "CommitteeNode timestamp test" and the actual changes apply this fix across multiple test files in different components (CommitteeNode, common_test, milestone, user, and member tests), the description accurately characterizes the nature of the changes. The description is not off-topic or unrelated; it simply narrows the scope to one of the affected files rather than comprehensively listing all changes.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix/equality-checks-for-floats

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@kasya kasya marked this pull request as ready for review October 29, 2025 02:36
@arkid15r arkid15r enabled auto-merge October 29, 2025 02:52
arkid15r
arkid15r previously approved these changes Oct 29, 2025
Copy link
Collaborator

@arkid15r arkid15r left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@arkid15r arkid15r disabled auto-merge October 29, 2025 02:54
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
backend/tests/apps/owasp/management/commands/owasp_create_project_metadata_file_test.py (1)

51-51: Follow pytest.approx convention for better readability.

While the current usage works correctly, the idiomatic pytest pattern is to wrap the expected value rather than the actual value. This improves readability by making it clear which value is the reference.

Apply this diff to follow pytest conventions:

-        assert pytest.approx(metadata["level"]) == 3.5
+        assert metadata["level"] == pytest.approx(3.5)
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 4311ecd and 32f94e2.

📒 Files selected for processing (6)
  • backend/tests/apps/owasp/management/commands/owasp_create_project_metadata_file_test.py (1 hunks)
  • backend/tests/apps/sitemap/views/base_test.py (2 hunks)
  • backend/tests/apps/sitemap/views/chapter_test.py (2 hunks)
  • backend/tests/apps/sitemap/views/committee_test.py (2 hunks)
  • backend/tests/apps/sitemap/views/repository_test.py (2 hunks)
  • backend/tests/apps/sitemap/views/static_test.py (1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
backend/tests/apps/sitemap/views/committee_test.py (1)
backend/apps/sitemap/views/base.py (1)
  • priority (50-52)
backend/tests/apps/sitemap/views/base_test.py (3)
backend/tests/apps/sitemap/views/static_test.py (1)
  • sitemap (11-12)
backend/apps/sitemap/views/base.py (2)
  • get_static_priority (26-32)
  • priority (50-52)
backend/apps/sitemap/views/static.py (1)
  • priority (49-51)
backend/tests/apps/sitemap/views/chapter_test.py (2)
backend/apps/sitemap/views/base.py (1)
  • priority (50-52)
backend/apps/sitemap/views/static.py (1)
  • priority (49-51)
backend/tests/apps/sitemap/views/repository_test.py (1)
backend/apps/sitemap/views/base.py (1)
  • priority (50-52)
backend/tests/apps/sitemap/views/static_test.py (1)
backend/apps/sitemap/views/static.py (1)
  • priority (49-51)
⏰ 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). (4)
  • GitHub Check: Run frontend e2e tests
  • GitHub Check: Run backend tests
  • GitHub Check: Run frontend unit tests
  • GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (5)
backend/tests/apps/sitemap/views/repository_test.py (1)

3-3: LGTM! Proper use of pytest.approx for floating point comparison.

The changes correctly replace the direct floating point equality check with pytest.approx(), which is the recommended pytest approach for comparing floating point values. This addresses SonarQube rule S1244 appropriately.

Also applies to: 61-61

backend/tests/apps/sitemap/views/committee_test.py (1)

3-3: LGTM! Consistent floating point comparison fix.

The changes properly use pytest.approx() for the floating point comparison, maintaining consistency with the other sitemap test files and addressing the SonarQube rule S1244.

Also applies to: 51-51

backend/tests/apps/sitemap/views/base_test.py (1)

24-25: LGTM! Comprehensive floating point comparison updates.

All floating point comparisons in this test file have been properly updated to use pytest.approx(), covering tests for both known and unknown paths as well as the priority method. The changes are consistent and correct.

Also applies to: 28-28, 53-53

backend/tests/apps/sitemap/views/chapter_test.py (1)

3-3: LGTM! Proper floating point comparison implementation.

The changes correctly implement pytest.approx() for the floating point comparison, consistent with the pattern applied across all sitemap tests.

Also applies to: 54-54

backend/tests/apps/sitemap/views/static_test.py (1)

61-61: LGTM! Proper floating point comparison in iterative test.

The change correctly uses pytest.approx() for comparing floating point priority values within the loop, ensuring accurate comparisons for all static routes.

@sonarqubecloud
Copy link

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 32f94e2 and 226504d.

📒 Files selected for processing (6)
  • backend/tests/apps/github/api/internal/nodes/milestone_test.py (3 hunks)
  • backend/tests/apps/github/api/internal/nodes/user_test.py (3 hunks)
  • backend/tests/apps/owasp/api/internal/nodes/committee_test.py (2 hunks)
  • backend/tests/apps/owasp/api/internal/nodes/common_test.py (2 hunks)
  • backend/tests/apps/owasp/management/commands/owasp_create_project_metadata_file_test.py (1 hunks)
  • backend/tests/apps/slack/admin/member_test.py (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • backend/tests/apps/owasp/management/commands/owasp_create_project_metadata_file_test.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • backend/tests/apps/owasp/api/internal/nodes/common_test.py
⏰ 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). (4)
  • GitHub Check: Run frontend e2e tests
  • GitHub Check: Run frontend unit tests
  • GitHub Check: Run backend tests
  • GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/tests/apps/slack/admin/member_test.py (1)

33-33: Remove this review comment—the change is intentional and related to the PR scope.

The modification removes pytest.approx() from a string assertion, which is directly related to fixing SonarQube rule S1244 (floating-point equality checks). The pytest.approx() function is for comparing floating-point numbers with tolerance, not for string comparisons, making its removal the correct fix. The leading space in " assigned user for {mock_member}." is pre-existing code and was not introduced by this change.

Likely an incorrect or invalid review comment.

@arkid15r arkid15r enabled auto-merge October 29, 2025 03:14
@arkid15r arkid15r added this pull request to the merge queue Oct 29, 2025
Merged via the queue into main with commit 8587763 Oct 29, 2025
26 checks passed
@arkid15r arkid15r deleted the fix/equality-checks-for-floats branch October 29, 2025 03:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants