test tests #5426
test tests #5426
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unittests/dataset_tests.py (1)
10-10: Remove unused importThe
uuidmodule is imported but never used in the code.-import uuid🧰 Tools
🪛 Ruff (0.8.2)
10-10:
uuidimported but unusedRemove unused import:
uuid(F401)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unittests/dataset_tests.py(4 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/unittests/dataset_tests.py
10-10: uuid imported but unused
Remove unused import: uuid
(F401)
🔇 Additional comments (3)
tests/unittests/dataset_tests.py (3)
2573-2575: Ensure unique dataset names using datetimeGood practice to use datetime for generating unique dataset names and explicitly saving the dataset after creation.
2578-2578: Maintain consistency in dataset namingGood practice to use the same datetime-based naming pattern for the cloned dataset.
2604-2605: Improve test assertions using values() methodGood refactor to use the values() method for retrieving field contents consistently.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
fiftyone/core/singletons.py (1)
339-342: Enhance the warning message for better debugging.The warning message is incomplete and could provide more context. Consider using f-strings for better readability.
- logging.warning( - "Cannot sync frames to corresponding sample collection `%s` ", - collection_name, - ) + logging.warning( + f"Cannot sync frames to corresponding sample collection `{collection_name}` " + f"as it was not found in {cls.__name__}._instances" + )fiftyone/core/dataset.py (2)
3377-3383: Improve error handling for MongoDB insert operation.The error handling for
insert_manyhas been enhanced to properly capture and handle MongoDB bulk write errors.Consider adding a more descriptive error message that includes the number of samples that failed to insert:
- msg = bwe.details["writeErrors"][0]["errmsg"] - raise ValueError(msg) from bwe + msg = bwe.details["writeErrors"][0]["errmsg"] + raise ValueError( + f"Failed to insert samples. First error: {msg}. " + f"Number of errors: {len(bwe.details['writeErrors'])}" + ) from bwe
3388-3390: Add validation to ensure all samples were inserted.The code now validates that all samples were successfully inserted by comparing the number of inserted IDs to the number of samples.
Consider providing more context in the error message:
- raise BulkWriteError("Failed to insert all samples") + raise BulkWriteError( + f"Failed to insert all samples: expected {len(samples)} " + f"but inserted {len(new_ids)}" + )
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
fiftyone/core/dataset.py(1 hunks)fiftyone/core/singletons.py(2 hunks)tests/unittests/dataset_tests.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/unittests/dataset_tests.py
🔇 Additional comments (2)
fiftyone/core/singletons.py (1)
8-8: LGTM!The logging import is appropriately placed with other standard library imports.
fiftyone/core/dataset.py (1)
3391-3396: LGTM: Sample document creation and video frame handling.The code correctly:
- Assigns new IDs to the sample documents
- Creates backing documents for samples
- Handles video frame saving for video samples
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/unittests/dataset_tests.py (3)
2573-2577: Consider removing or converting print statements to a logger.
Printing datasets and samples is helpful for debugging, but using test assertions or logging might be more consistent with typical testing practices.
2586-2588: Prefer structured assertions over printing.
Relying on print statements alone to verify correctness might hide issues; assertions provide clearer test outcomes.
2608-2610: Improve test clarity by using assertions.
Instead of manually inspecting prints, consider verifying thatdataset2contains the expected samples, with or without fields, to ensure correctness.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unittests/dataset_tests.py(3 hunks)
🔇 Additional comments (4)
tests/unittests/dataset_tests.py (4)
2580-2582: Unique dataset naming approach is acceptable.
Generating random names viauuid.uuid4()can minimize collisions and works well for ephemeral tests.
2602-2604: Fine usage of ephemeral dataset clones.
Cloning with unique UUID names avoids conflicts when running tests in parallel.
2605-2606: Looks good.
Capturingcreated_atandlast_modified_atfor subsequent comparisons is a standard approach in these tests.
2607-2607: Verify consistency of new IDs.
When adding a collection withnew_ids=True, ensure any references are handled properly so downstream steps remain consistent.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/unittests/dataset_tests.py (1)
2576-2577: Consider removing debug print statements.While print statements are helpful during development, they should be removed or replaced with proper logging for production code.
- print("dataset1") - print([s for s in dataset1.iter_samples()]) - print("exact clone") - print([s for s in dataset.iter_samples()]) - print("merged dataset1") - print([s for s in dataset.iter_samples()]) - print("dataset2 exact clone") - print([s for s in dataset1.iter_samples()]) - print("merge view (ds with no foo)") - print([s for s in dataset2.iter_samples()])Also applies to: 2581-2582, 2586-2587, 2603-2604, 2608-2609
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/unittests/dataset_tests.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.11)
- GitHub Check: test / test-python (ubuntu-latest-m, 3.10)
- GitHub Check: test / test-app
- GitHub Check: lint / eslint
- GitHub Check: e2e / test-e2e
- GitHub Check: build / build
- GitHub Check: build / changes
- GitHub Check: build
🔇 Additional comments (4)
tests/unittests/dataset_tests.py (4)
10-10: Import uuid module for generating unique identifiers.The addition of the uuid module import is appropriate for generating unique dataset names to prevent collisions in tests.
2573-2575: Use uuid for unique dataset names and ensure persistence.Good practice to:
- Use uuid.uuid4() for guaranteed unique dataset names
- Call save() immediately after creation to persist the dataset
2580-2580: Use uuid for clone names to prevent collisions.Good practice to use uuid.uuid4() when cloning datasets to ensure unique names.
Also applies to: 2602-2602
2605-2617: Test logic for dataset cloning and merging.The test logic appropriately verifies:
- Dataset creation timestamps
- Last modified timestamps
- Field values and counts
- Dataset uniqueness
What changes are proposed in this pull request?
(Please fill in changes proposed in this fix)
How is this patch tested? If it is not, please explain why.
(Details)
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
(Details in 1-2 sentences. You can just refer to another PR with a description
if this PR is part of a larger change.)
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
uuid.