Add dataset factory and canvas testing e2e guidance#7249
Add dataset factory and canvas testing e2e guidance#7249benjaminpkane wants to merge 5 commits intodevelopfrom
Conversation
benjaminpkane
added
testing
WalkthroughThis pull request adds 117 lines to the e2e-pw README with guidance for tests: use DatasetFactory.createBlankDataset (with schema and withSampleData) for FiftyOne datasets; derive stable sample Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e-pw/README.md`:
- Line 124: Replace the redundant phrase "blank PNG images" with "blank PNGs" in
the README where the dataset generation is described; locate the sentence
containing the exact text "blank PNG images" and update it to read "blank PNGs"
so the wording is tighter and consistent.
- Around line 195-206: The README overstates available signals; update the text
to either mention SampleCanvasAsserter.assert.is(type) as an additional
assertion or rephrase “only available signals” to “primary signals” (or similar)
and list hasCursor, hasScreenshot, and is so readers know assert.is(...) is also
supported; reference the SampleCanvasAsserter type and its methods assert.is,
assert.hasCursor, and assert.hasScreenshot when updating the wording.
- Around line 128-150: The snippet passed to DatasetFactory.createBlankDataset
calls indexToId(index) but does not import indexToId; add an import for
indexToId from the appropriate module (e.g., "src/shared/utils") at the top of
the file so the symbol is available when using createBlankDataset's
withSampleData callback; ensure the import line is added alongside the existing
import of DatasetFactory to prevent runtime/reference errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: e4aa7dc5-49a6-4a96-a3d6-1336465cab83
📒 Files selected for processing (1)
e2e-pw/README.md
| Since the canvas surface is opaque to the DOM, the only available signals for | ||
| assertions are **screenshots** and **cursor values**. Use these to verify that | ||
| an interaction had the expected effect. | ||
|
|
||
| ```ts | ||
| // Assert the CSS cursor at the current pointer position | ||
| await modal.sampleCanvas.assert.hasCursor("default"); | ||
| await modal.sampleCanvas.assert.hasCursor("nwse-resize"); | ||
|
|
||
| // Assert the canvas state via screenshot | ||
| await modal.sampleCanvas.assert.hasScreenshot("my-test-state.png"); | ||
| ``` |
There was a problem hiding this comment.
Soften or correct “only available signals” wording.
Line 195 says only cursor/screenshot assertions are available, but SampleCanvasAsserter also exposes is(type). Either document assert.is(...) or reword this as recommended primary signals to avoid misleading contributors.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e-pw/README.md` around lines 195 - 206, The README overstates available
signals; update the text to either mention SampleCanvasAsserter.assert.is(type)
as an additional assertion or rephrase “only available signals” to “primary
signals” (or similar) and list hasCursor, hasScreenshot, and is so readers know
assert.is(...) is also supported; reference the SampleCanvasAsserter type and
its methods assert.is, assert.hasCursor, and assert.hasScreenshot when updating
the wording.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
e2e-pw/README.md (3)
128-150:⚠️ Potential issue | 🟡 MinorAdd missing
indexToIdimport in the first code snippet.Line 140 uses
indexToId(index)but this snippet doesn't import it. While line 158 shows the import later, readers copying the first example will encounter a reference error.📦 Add the missing import
```ts import { DatasetFactory } from "src/shared/dataset-factory"; +import { indexToId } from "src/shared/utils"; await DatasetFactory.createBlankDataset({🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-pw/README.md` around lines 128 - 150, The example calls indexToId(index) but the snippet doesn't import it; update the first code sample so it imports indexToId from the same utils module as the later example (i.e., add an import for indexToId from "src/shared/utils") alongside the existing DatasetFactory import so the createBlankDataset example compiles and doesn't throw a ReferenceError.
198-200:⚠️ Potential issue | 🟡 MinorSoften or correct "only available signals" wording.
Line 198 states these are the "only available signals," but if
SampleCanvasAsserterexposes additional assertion methods likeassert.is(type), this claim is inaccurate. Either document all available assertions or rephrase to "primary signals" to avoid misleading contributors.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-pw/README.md` around lines 198 - 200, The README's claim that screenshots and cursor values are the "only available signals" is incorrect if SampleCanvasAsserter exposes other assertion helpers (e.g., SampleCanvasAsserter.assert.is(type)); update the sentence to either list all available assertion methods from SampleCanvasAsserter (including assert.is, etc.) or soften the wording to "primary signals" (e.g., "The primary signals available for assertions are screenshots and cursor values") so it no longer misrepresents available APIs; locate the sentence referencing "only available signals" and change it accordingly.
124-124:⚠️ Potential issue | 🟡 MinorTighten redundant wording.
Use "blank PNGs" instead of "blank PNG images" per the static analysis hint.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e-pw/README.md` at line 124, Replace the redundant phrase "blank PNG images" with the tighter "blank PNGs" in the README text (look for the sentence containing "dataset. It generates blank PNG images, inserts samples directly into the"); update that phrase to "blank PNGs" to satisfy the static analysis suggestion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@e2e-pw/README.md`:
- Around line 152-156: Clarify whether stable `_id`s are created automatically
or must be assigned in the sample setup: update the README text around the
`withSampleData` example to state that stable IDs are assigned only when you
explicitly set `_id` (as shown with `withSampleData` using
`createId(indexToId(index))`), and note that if your framework/tool also
auto-generates IDs those can be overridden by providing an `_id` value;
reference `withSampleData`, `_id`, `createId`, and `indexToId` in the sentence
so readers know exactly where to set or override stable IDs.
---
Duplicate comments:
In `@e2e-pw/README.md`:
- Around line 128-150: The example calls indexToId(index) but the snippet
doesn't import it; update the first code sample so it imports indexToId from the
same utils module as the later example (i.e., add an import for indexToId from
"src/shared/utils") alongside the existing DatasetFactory import so the
createBlankDataset example compiles and doesn't throw a ReferenceError.
- Around line 198-200: The README's claim that screenshots and cursor values are
the "only available signals" is incorrect if SampleCanvasAsserter exposes other
assertion helpers (e.g., SampleCanvasAsserter.assert.is(type)); update the
sentence to either list all available assertion methods from
SampleCanvasAsserter (including assert.is, etc.) or soften the wording to
"primary signals" (e.g., "The primary signals available for assertions are
screenshots and cursor values") so it no longer misrepresents available APIs;
locate the sentence referencing "only available signals" and change it
accordingly.
- Line 124: Replace the redundant phrase "blank PNG images" with the tighter
"blank PNGs" in the README text (look for the sentence containing "dataset. It
generates blank PNG images, inserts samples directly into the"); update that
phrase to "blank PNGs" to satisfy the static analysis suggestion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 96096475-47a4-448b-ab5f-bc28cb606d06
📒 Files selected for processing (1)
e2e-pw/README.md
2 participants
What changes are proposed in this pull request?
Adds dataset factory and canvas testing sections to the E2E README
How is this patch tested? If it is not, please explain why.
N/A
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit