-
Notifications
You must be signed in to change notification settings - Fork 591
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Render summary fields in modal sidebar #4851
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -126,6 +126,18 @@ class SidebarAsserter { | |||||||||||
); | ||||||||||||
} | ||||||||||||
|
||||||||||||
async verifyObject(key: string, obj: { [key: string]: string }) { | ||||||||||||
const locator = this.modalSidebarPom.getSidebarEntry(key); | ||||||||||||
|
||||||||||||
for (const k in obj) { | ||||||||||||
const v = obj[k]; | ||||||||||||
const entry = locator.getByTestId(`key-value-${k}-${v}`); | ||||||||||||
Comment on lines
+132
to
+134
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Use Using Apply this diff to implement the change: - for (const k in obj) {
- const v = obj[k];
+ for (const [k, v] of Object.entries(obj)) { 📝 Committable suggestion
Suggested change
|
||||||||||||
|
||||||||||||
await expect(entry.getByTestId(`key-${k}`)).toHaveText(k); | ||||||||||||
await expect(entry.getByTestId(`value-${v}`)).toHaveText(v); | ||||||||||||
} | ||||||||||||
} | ||||||||||||
|
||||||||||||
async verifyLabelTagCount(count: number) { | ||||||||||||
await this.modalSidebarPom.page.waitForFunction( | ||||||||||||
(count_) => { | ||||||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,70 @@ | ||
import { test as base } from "src/oss/fixtures"; | ||
import { GridPom } from "src/oss/poms/grid"; | ||
import { ModalPom } from "src/oss/poms/modal"; | ||
import { getUniqueDatasetNameWithPrefix } from "src/oss/utils"; | ||
|
||
const test = base.extend<{ grid: GridPom; modal: ModalPom }>({ | ||
grid: async ({ page, eventUtils }, use) => { | ||
await use(new GridPom(page, eventUtils)); | ||
}, | ||
modal: async ({ page, eventUtils }, use) => { | ||
await use(new ModalPom(page, eventUtils)); | ||
}, | ||
}); | ||
|
||
const datasetName = getUniqueDatasetNameWithPrefix("summary-fields"); | ||
|
||
test.describe("summary fields", () => { | ||
test.beforeAll(async ({ fiftyoneLoader }) => { | ||
await fiftyoneLoader.executePythonCode(` | ||
import fiftyone as fo | ||
|
||
dataset = fo.Dataset("${datasetName}") | ||
dataset.persistent = True | ||
dataset.add_sample( | ||
fo.Sample( | ||
filepath=f"image.png", | ||
summary=fo.DynamicEmbeddedDocument(one="two", three="four"), | ||
summaries=[ | ||
fo.DynamicEmbeddedDocument(five="six", seven="eight"), | ||
fo.DynamicEmbeddedDocument(nine="ten"), | ||
], | ||
) | ||
) | ||
dataset.app_config.sidebar_groups = [ | ||
fo.SidebarGroupDocument( | ||
name="summaries", paths=["summary", "summaries"], expanded=True | ||
) | ||
] | ||
dataset.save() | ||
dataset.add_dynamic_sample_fields() | ||
`); | ||
Comment on lines
+19
to
+41
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ensure safe interpolation of While interpolating Consider explicitly validating or sanitizing const sanitizedDatasetName = datasetName.replace(/[^a-zA-Z0-9_\-]/g, ""); And then use |
||
}); | ||
|
||
test("modal sidebar summary fields render", async ({ | ||
eventUtils, | ||
fiftyoneLoader, | ||
grid, | ||
modal, | ||
page, | ||
}) => { | ||
await fiftyoneLoader.waitUntilGridVisible(page, datasetName); | ||
await grid.openFirstSample(); | ||
await modal.waitForSampleLoadDomAttribute(true); | ||
await modal.sidebar.assert.verifyObject("summary", { | ||
one: "two", | ||
three: "four", | ||
}); | ||
const entryExpandPromise = eventUtils.getEventReceivedPromiseForPredicate( | ||
"animation-onRest", | ||
() => true | ||
); | ||
await modal.sidebar.clickFieldDropdown("summaries"); | ||
await entryExpandPromise; | ||
Comment on lines
+58
to
+63
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Refine event predicate to prevent flaky tests The predicate function Refine the predicate to ensure it matches the specific element or context: const entryExpandPromise = eventUtils.getEventReceivedPromiseForPredicate(
"animation-onRest",
(event) => event.target === expectedTargetElement
); Replace |
||
await modal.sidebar.assert.verifyObject("summaries", { | ||
five: "six", | ||
seven: "eight", | ||
nine: "ten", | ||
}); | ||
}); | ||
Comment on lines
+44
to
+69
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add cleanup logic to remove the dataset after tests The test creates a persistent dataset in the Implement an test.afterAll(async ({ fiftyoneLoader }) => {
await fiftyoneLoader.executePythonCode(`
import fiftyone as fo
if fo.dataset_exists("${datasetName}"):
dataset = fo.load_dataset("${datasetName}")
dataset.delete()
`);
}); |
||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
(nit: although
FoPrimitive
would probably make it less confusing elsewhere)