Add unit tests for native bucket scanner, also check it in data/health #8528
Conversation
📝 Walkthrough""" WalkthroughThis change introduces a comprehensive test suite for the Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
fm3
changed the title
![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 (3)
test/backend/BucketScannerTestSuite.scala (3)
33-49: Update comment to reflect all values in arrayThe test logic is good, but the comment on line 35 doesn't fully match the array contents.
- // little endian uint16 representation of 2, 4, 500, 500 + // little endian uint16 representation of 2, 4, 500, 500, 0The array includes an additional zero value at the end (bytes 8-9), which is correctly tested for in the assertions but not mentioned in the comment.
51-66: Good test coverage for voxel counting functionalityThis test effectively verifies both positive and negative cases for segment voxel counting.
There's a minor discrepancy in the comment on line 53:
- // little endian uint16 representation of 2, 4, 500, 500 + // little endian uint32 representation of 2, 4, 500, 500The test is correctly using uint32, but the comment says uint16.
68-94: Add clarifying comments to the bounding box testThe bounding box test is well-implemented but fairly complex. Consider adding more explanatory comments to improve readability.
Specific areas that would benefit from additional comments:
- The calculation of
bytesPerBucket(lines 70-73) - explain why you're calculating it this way- The setting of specific bytes (lines 74-75) - explain what these positions represent in 3D space
- The meaning of the many parameters passed to
extendSegmentBoundingBox(lines 77-92) - particularly the Int.Min/MaxValue parameters and what they representFor example:
// Set segment ID 1 at positions that will result in a bounding box from (5,1,0) to (8,1,0) array(ElementClass.bytesPerElement(elementClass) * (DataLayer.bucketLength + 5)) = 1 array(ElementClass.bytesPerElement(elementClass) * (DataLayer.bucketLength + 8)) = 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/backend/BucketScannerTestSuite.scala(1 hunks)webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/Application.scala(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build-test-e2e-deploy
🔇 Additional comments (4)
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/Application.scala (1)
5-9: Health endpoint integration looks goodThe integration of the NativeBucketScanner test into the health endpoint is well implemented. Using the
toFox ?~>pattern maintains consistency with the existing error handling approach in this method.Also applies to: 29-29
test/backend/BucketScannerTestSuite.scala (3)
7-19: Well-structured test for uint16 segment ID collectionThe test for collecting segment IDs from a uint16 byte array is well-structured and clearly demonstrates the expected functionality of the NativeBucketScanner.
21-31: Effective test for uint32 element class compatibilityGood test that verifies the scanner works correctly with uint32 values, demonstrating element class flexibility.
7-96: Comprehensive test coverage for NativeBucketScannerThe test suite provides excellent coverage of the NativeBucketScanner's functionality, including segment ID collection, zero-skipping, voxel counting, and bounding box calculations. The tests are well-structured, independent, and verify both positive and negative cases where appropriate.
This implementation aligns perfectly with the PR objective of adding unit tests for the NativeBucketScanner class. The tests complement the runtime verification added to the health check endpoint.
webknossos-datastore/app/com/scalableminds/webknossos/datastore/controllers/Application.scala
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
.github/workflows/build_test_deploy.yml (1)
217-234: DRY up repeated smoke-test steps using a matrix
The three smoke-test steps differ only by URL. You can reduce duplication by leveraging a matrix strategy:- - name: Run webknossos smoke test - uses: ./.github/actions/retry - with: - run: curl -v --fail-with-body "http://localhost:9000/api/health" - retries: 20 - retry_delay_seconds: 5 - - name: Run webknossos-datastore smoke test - uses: ./.github/actions/retry - with: - run: curl -v --fail-with-body "http://localhost:9090/data/health" - retries: 20 - retry_delay_seconds: 5 - - name: Run webknossos-tracingstore smoke test - uses: ./.github/actions/retry - with: - run: curl -v --fail-with-body "http://localhost:9050/tracings/health" - retries: 20 - retry_delay_seconds: 5 + - name: Run service health smoke tests + uses: ./.github/actions/retry + strategy: + matrix: + service: + - {name: webknossos, url: "http://localhost:9000/api/health"} + - {name: datastore, url: "http://localhost:9090/data/health"} + - {name: tracingstore, url: "http://localhost:9050/tracings/health"} + with: + run: | + echo "Testing ${{ matrix.service.name }} at ${{ matrix.service.url }}" + curl -v --fail-with-body "${{ matrix.service.url }}" + retries: 20 + retry_delay_seconds: 5This will make it easier to add or remove services later without copying the same step multiple times.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/build_test_deploy.yml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build-smoketest-push
- GitHub Check: backend-tests
🔇 Additional comments (2)
.github/workflows/build_test_deploy.yml (2)
99-99: Upgrade CI runner to Ubuntu 24.04
Updating thebuild-smoketest-pushjob fromubuntu-22.04toubuntu-24.04brings newer system libraries and security patches. Please verify that all required tools (Java 21, SBT,libdraco-dev,libblosc1, etc.) remain fully compatible in this environment before merging.
220-220: Switch tocurl --fail-with-bodyfor richer failure output
Replacing--failwith--fail-with-bodywill include the HTTP response body on failures, which is invaluable for diagnosing health‐check errors. Confirm that the defaultcurlon Ubuntu 24.04 (requires ≥ 7.76) supports this flag so the smoke tests don’t break unexpectedly.Also applies to: 226-226, 232-232
2 participants
Steps to test:
Issues