Fix nested-dynamic-groups-as-video bug #4548
Conversation
WalkthroughThe updates modify various e2e testing components, including adding new methods for better validation and changing existing methods to improve consistency and functionality. Changes encompass timer configuration based on an environment variable, new and updated methods in asserters, and test scenario enhancements. Additionally, a new test for nested dynamic groups is introduced. Changes
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
sashankaryal
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: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/packages/state/src/recoil/dynamicGroups.ts (2 hunks)
Additional context used
Path-based instructions (1)
app/packages/state/src/recoil/dynamicGroups.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Additional comments not posted (3)
app/packages/state/src/recoil/dynamicGroups.ts (3)
78-78: Update filter type definition to support multiple slices.The filter object now supports both single
sliceand multiplesliceswhich enhances flexibility.
87-88: Useslicedirectly in filter.The filter now directly uses
slicewhich simplifies the code.
95-97: Add conditional check for group slices.The code now conditionally sets
params.filter.group.slicesto an array containingsliceifhasGroupSlicesis true. This ensures that the filter criteria are correctly applied.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
e2e-pw/package.jsonis excluded by!**/*.jsone2e-pw/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
Files selected for processing (5)
- app/packages/looker/src/lookers/imavid/controller.ts (1 hunks)
- e2e-pw/playwright.config.ts (1 hunks)
- e2e-pw/src/oss/poms/grid/index.ts (1 hunks)
- e2e-pw/src/oss/poms/modal/modal-sidebar.ts (1 hunks)
- e2e-pw/src/oss/specs/groups/nested-dynamic-groups.spec.ts (1 hunks)
Additional context used
Path-based instructions (5)
e2e-pw/playwright.config.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/poms/modal/modal-sidebar.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/poms/grid/index.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/specs/groups/nested-dynamic-groups.spec.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/looker/src/lookers/imavid/controller.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Additional comments not posted (5)
e2e-pw/playwright.config.ts (1)
12-12: Ensure timeout configuration is appropriate.The timeout configuration has been modified to extend the timeout to 10 minutes if
USE_DEV_BUILDis set. This change seems appropriate for development builds, but ensure that the longer timeout is necessary and won't mask potential issues during development.e2e-pw/src/oss/poms/modal/modal-sidebar.ts (1)
64-64: New methodwaitUntilSidebarEntryTextEqualslooks good.The new method
waitUntilSidebarEntryTextEqualsis well-implemented. It useswaitForFunctionto wait for a sidebar entry text to match a specified value within a 5000 milliseconds timeout.e2e-pw/src/oss/poms/grid/index.ts (1)
102-102: New methodnthSampleHasTagValuelooks good.The new method
nthSampleHasTagValueis well-implemented. It checks if a specific tag value exists for a sample at a given index within a grid.e2e-pw/src/oss/specs/groups/nested-dynamic-groups.spec.ts (1)
1-1: New test filenested-dynamic-groups.spec.tslooks good.The new test file
nested-dynamic-groups.spec.tsis well-implemented. It includes setups for grids, modals, and sidebars, along with the creation and validation of nested dynamic groups within a dataset. The tests are comprehensive and cover various scenarios.app/packages/looker/src/lookers/imavid/controller.ts (1)
122-122: Verify the correctness of the argument change infetchMore.The change from subtracting
1to subtracting2in thefetchMoremethod might affect the pagination logic. Ensure that this change is intentional and correctly adjusts the range of frames fetched.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
e2e-pw/package.jsonis excluded by!**/*.json
Files selected for processing (4)
- e2e-pw/src/oss/poms/grid/index.ts (2 hunks)
- e2e-pw/src/oss/poms/modal/index.ts (1 hunks)
- e2e-pw/src/oss/poms/operators/operators-prompt.ts (1 hunks)
- e2e-pw/src/shared/media-factory/video.ts (1 hunks)
Files skipped from review due to trivial changes (1)
- e2e-pw/src/shared/media-factory/video.ts
Additional context used
Path-based instructions (3)
e2e-pw/src/oss/poms/operators/operators-prompt.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/poms/grid/index.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/poms/modal/index.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Additional comments not posted (3)
e2e-pw/src/oss/poms/operators/operators-prompt.ts (1)
60-60: Use oftoBeHiddenis appropriateThe change to use
toBeHiddeninstead ofnot.toBeVisibleis appropriate as it is more explicit and aligns with best practices for checking visibility.e2e-pw/src/oss/poms/grid/index.ts (1)
101-109: New methodnthSampleHasTagValuelooks goodThe new method
nthSampleHasTagValuecorrectly retrieves the tag element for the nth looker and asserts its text value. This method is well-implemented.e2e-pw/src/oss/poms/modal/index.ts (1)
246-246: Use oftoHaveTextinverifySelectionCountis appropriateThe change to use
toHaveTextfor text verification inverifySelectionCountmethod is appropriate and aligns with best practices for text content assertions.
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- e2e-pw/src/oss/specs/groups/nested-dynamic-groups.spec.ts (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- e2e-pw/src/oss/specs/groups/nested-dynamic-groups.spec.ts
sashankaryal
force-pushed
the
bugfix/dynamic-groups-slices
branch
from
38e7314
to
8ad2fcd
Compare
| "nested-dynamic-groups" | ||
| ); | ||
|
|
||
| function* orderGenerator() { |
|
|
||
| samples = [] | ||
|
|
||
| image_paths = ${JSON.stringify(imagePaths)} |
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (2)
e2e-pw/package.jsonis excluded by!**/*.jsone2e-pw/yarn.lockis excluded by!**/yarn.lock,!**/*.lock,!**/*.lock
Files selected for processing (9)
- app/packages/looker/src/lookers/imavid/controller.ts (1 hunks)
- app/packages/state/src/recoil/dynamicGroups.ts (2 hunks)
- e2e-pw/playwright.config.ts (1 hunks)
- e2e-pw/src/oss/poms/grid/index.ts (2 hunks)
- e2e-pw/src/oss/poms/modal/index.ts (1 hunks)
- e2e-pw/src/oss/poms/modal/modal-sidebar.ts (1 hunks)
- e2e-pw/src/oss/poms/operators/operators-prompt.ts (1 hunks)
- e2e-pw/src/oss/specs/groups/nested-dynamic-groups.spec.ts (1 hunks)
- e2e-pw/src/shared/media-factory/video.ts (1 hunks)
Files skipped from review as they are similar to previous changes (9)
- app/packages/looker/src/lookers/imavid/controller.ts
- app/packages/state/src/recoil/dynamicGroups.ts
- e2e-pw/playwright.config.ts
- e2e-pw/src/oss/poms/grid/index.ts
- e2e-pw/src/oss/poms/modal/index.ts
- e2e-pw/src/oss/poms/modal/modal-sidebar.ts
- e2e-pw/src/oss/poms/operators/operators-prompt.ts
- e2e-pw/src/oss/specs/groups/nested-dynamic-groups.spec.ts
- e2e-pw/src/shared/media-factory/video.ts
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (2)
- e2e-pw/src/oss/poms/sidebar.ts (1 hunks)
- e2e-pw/src/oss/specs/regression-tests/sidebar/sidebar-datetime.spec.ts (1 hunks)
Additional context used
Path-based instructions (2)
e2e-pw/src/oss/specs/regression-tests/sidebar/sidebar-datetime.spec.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/poms/sidebar.ts (1)
Pattern
**/*.{ts,tsx}: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
Additional comments not posted (2)
e2e-pw/src/oss/specs/regression-tests/sidebar/sidebar-datetime.spec.ts (1)
130-130: Verify parameter change forchangeSliderStartValue.The parameters for
changeSliderStartValuehave been updated to59.000and0.000. Ensure these values are in the correct format and consistent with the expected input for this function.e2e-pw/src/oss/poms/sidebar.ts (1)
91-93: Verify variable name changes inchangeSliderStartValue.The variables
sliderStartandsliderMidPointhave been introduced. Ensure these changes maintain the expected behavior of the function.
What changes are proposed in this pull request?
When expanding a sample in a nested dynamic group when imavid is active, the first sample is always displayed. This PR fixes that.
Before
After
Release Notes
Is this a user-facing change that should be mentioned in the release notes?
notes for FiftyOne users.
What areas of FiftyOne does this PR affect?
fiftyonePython library changesSummary by CodeRabbit
New Features
Enhancements
Bug Fixes
Chores