Skip to content

Move some TestImageDatasetExport to TestDatasetExport - #1633

Merged
MalteEbner merged 3 commits into
mainfrom
malte-lig-9972-export-test-refactor
Jul 14, 2026
Merged

Move some TestImageDatasetExport to TestDatasetExport#1633
MalteEbner merged 3 commits into
mainfrom
malte-lig-9972-export-test-refactor

Conversation

@MalteEbner

@MalteEbner MalteEbner commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

What has changed and why

Move tests from TestImageDatasetExport to the new class TestDatasetExport. Split the latter across different files to keep the files small.

TestImageDatasetExport now only tests sample_to_image and that it is used correctly.

Review by Codex 5.5 extra high

image

Move the format-contract tests (COCO/YOLO/Pascal/captions output correctness,
category sorting, mask handling, path defaults) from TestImageDatasetExport into
a new TestDatasetExport, driving DatasetExport directly with the image mapping as
substrate. These assert behavior shared by every sample type, so they belong to
the base.

Slim test_image_dataset_export.py to the image-specific delta: unit tests for the
image_sample_to_image mapping (absolute path for COCO, relative name for
YOLO/Pascal) plus one smoke that ImageDataset.export() binds it.

No behavior change; coverage preserved. An upcoming VideoFrameDatasetExport only
needs its own small mapping tests, not a copy of the format suite.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@MalteEbner
MalteEbner requested a review from a team as a code owner July 13, 2026 12:04
@coderabbitai

coderabbitai Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR reorganizes image dataset export tests and adds dedicated coverage for COCO captions, COCO detections and segmentation masks, YOLO detections, and Pascal VOC masks. Image-specific tests now focus on query forwarding and absolute or relative filename mapping.

Changes

Dataset export test coverage

Layer / File(s) Summary
Detection export coverage
lightly_studio/tests/export/test_dataset_export__coco_object_detections.py, lightly_studio/tests/export/test_dataset_export__yolo_object_detections.py
COCO and YOLO tests cover filtering, output paths, categories, annotations, confidence scores, empty annotations, and annotation collection selection.
Caption and COCO segmentation coverage
lightly_studio/tests/export/test_dataset_export__coco_captions.py, lightly_studio/tests/export/test_dataset_export__coco_segmentation_masks.py
Tests validate COCO caption payloads, default and string output paths, segmentation fields, and omission of annotations with missing masks.
Pascal VOC mask coverage
lightly_studio/tests/export/test_dataset_export__pascal_voc.py
Tests validate class mappings and pixel masks for background labels, multiple classes, disjoint regions, and multiple images.
Image query and filename wiring
lightly_studio/tests/export/test_image_dataset_export.py
Image export tests validate query forwarding, absolute paths, relative basenames, and preserved dimensions.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

Suggested reviewers: horatiualmasan, lukas-lightly

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Description check ⚠️ Warning It summarizes the refactor, but it omits the required testing and CHANGELOG sections from the template. Add a testing section with the commands and results used, and include the CHANGELOG.md update status.
✅ Passed checks (4 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title accurately reflects the main change: moving export tests from TestImageDatasetExport to TestDatasetExport.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch malte-lig-9972-export-test-refactor

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.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e04f038de8

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread lightly_studio/tests/export/test_image_dataset_export.py Outdated
Comment thread lightly_studio/tests/export/test_dataset_export.py Outdated
Comment thread lightly_studio/tests/export/test_dataset_export.py Outdated
@MalteEbner MalteEbner changed the title Refactor export tests along the DatasetExport/ImageDatasetExport seam Move some TestImageDatasetExport to TestDatasetExport Jul 13, 2026
MalteEbner and others added 2 commits July 13, 2026 15:25
Split the DatasetExport format tests into per-format files
(test_dataset_export__coco_object_detections.py, __yolo_object_detections.py,
__coco_captions.py, __coco_segmentation_masks.py, __pascal_voc.py), matching the
test_<subject>__<topic>.py convention used in tests/core/image. The shared
DatasetExport factory moves to tests/export/helpers.py.

Restore query-forwarding coverage in the ImageDataset.export() smoke test, and
drop the "bind"/"substrate" wording from docstrings.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
build_dataset_export only re-did what ImageDatasetExport already does (bind the
image sample-to-image mapping), so instantiate ImageDatasetExport directly in the
format tests and remove tests/export/helpers.py.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Comment thread lightly_studio/tests/export/test_image_dataset_export.py
@MalteEbner

Copy link
Copy Markdown
Contributor Author

/review

@horatiualmasan horatiualmasan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@MalteEbner
MalteEbner enabled auto-merge July 14, 2026 10:23
@MalteEbner
MalteEbner added this pull request to the merge queue Jul 14, 2026
Merged via the queue into main with commit 231a13e Jul 14, 2026
23 checks passed
@MalteEbner
MalteEbner deleted the malte-lig-9972-export-test-refactor branch July 14, 2026 12:33
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