Skip to content

Conversation

@alphaprinz
Copy link
Contributor

@alphaprinz alphaprinz commented Nov 19, 2025

Describe the Problem

The 'health should test notification storage' in test_nc_health.js assumed vm test had enough free space.
This assumption was broken as of late, so I don't assume that anymore.

Explain the Changes

  1. Remove 'above' requirement for test.

Issues: Fixed #xxx / Gap #xxx

Testing Instructions:

  • Doc added/updated
  • Tests added

Summary by CodeRabbit

  • Tests
    • Improved notification storage test assertions for better validation.
    • Enhanced test diagnostics with additional filesystem logging.

@coderabbitai
Copy link

coderabbitai bot commented Nov 19, 2025

Walkthrough

A test file for notification health checks was modified to relax an exact string assertion into a substring check and add diagnostic logging. The assertion changed from comparing a result directly to the string 'above threshold' to checking if the result contains the substring ' threshold'.

Changes

Cohort / File(s) Change Summary
Test assertion relaxation and debug logging
src/test/integration_tests/nc/cli/test_nc_health.js
Modified notification storage test to use substring matching (indexOf > -1) instead of exact string comparison for the result value. Added console logging to print PSA filesystem statistics for NOTIFICATION_LOG_DIR directory prior to assertion.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

  • Verify that the substring check ' threshold' is appropriate and doesn't mask potential false positives
  • Confirm the console.log addition is temporary or intentional for debugging purposes

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title accurately describes the main change: modifying a test to not assume sufficient free filesystem space, matching the code change that replaces exact assertion with substring check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

@alphaprinz alphaprinz changed the title notification - nc test - don't assume test vm has enough fs free notification - nc test - don't assume test vm has enough free space in FS Nov 20, 2025
@alphaprinz alphaprinz merged commit 8ba55a8 into noobaa:master Nov 20, 2025
29 of 30 checks passed
alphaprinz added a commit to alphaprinz/noobaa-core that referenced this pull request Nov 20, 2025
alphaprinz added a commit to alphaprinz/noobaa-core that referenced this pull request Nov 20, 2025
alphaprinz added a commit to alphaprinz/noobaa-core that referenced this pull request Nov 20, 2025
alphaprinz added a commit to alphaprinz/noobaa-core that referenced this pull request Nov 20, 2025
alphaprinz added a commit to alphaprinz/noobaa-core that referenced this pull request Nov 20, 2025
liranmauda pushed a commit to alphaprinz/noobaa-core that referenced this pull request Nov 25, 2025
liranmauda pushed a commit to alphaprinz/noobaa-core that referenced this pull request Dec 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants