Skip to content
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.

Sign up for GitHub

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

Jump to bottom

kie-issues#1162: Add DMN Boxed Filter expression tests #2308

Merged
merged 27 commits into from
Jun 27, 2024
Merged

kie-issues#1162: Add DMN Boxed Filter expression tests #2308

merged 27 commits into from
Jun 27, 2024

Conversation

jomarko
Copy link
Contributor

@jomarko jomarko commented May 9, 2024

@jomarko jomarko added the pr: wip PR is still under development label May 13, 2024
@jomarko jomarko marked this pull request as ready for review May 14, 2024 14:31
@jomarko jomarko marked this pull request as draft May 14, 2024 14:31
@jomarko jomarko marked this pull request as ready for review May 16, 2024 07:22
@jomarko jomarko removed the pr: wip PR is still under development label May 16, 2024
ljmotta
ljmotta reviewed May 20, 2024
View reviewed changes
Copy link
Contributor

@ljmotta ljmotta left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@jomarko Thanks for this PR. I've made some comments inline. Also I've relized it's missing a test on the resize.spec.ts file. Please add one decription block there, we need to check the resize there.

packages/boxed-expression-component/tests/e2e/boxedExpressions/boxedExpressionHeader.spec.ts Outdated Show resolved Hide resolved
packages/boxed-expression-component/tests/e2e/boxedExpressions/filter/populate.spec.ts Outdated Show resolved Hide resolved
packages/boxed-expression-component/tests/e2e/boxedExpressions/filter/populate.spec.ts Outdated Show resolved Hide resolved
packages/boxed-expression-component/stories/boxedExpressions/Filter/Filter.stories.tsx Outdated Show resolved Hide resolved
packages/boxed-expression-component/tests/e2e/boxedExpressions/filter/populate.spec.ts Outdated Show resolved Hide resolved
packages/boxed-expression-component/tests/e2e/boxedExpressions/filter/populate.spec.ts Outdated Show resolved Hide resolved
packages/boxed-expression-component/tests/e2e/__fixtures__/boxedExpression.ts Outdated Show resolved Hide resolved
@jomarko jomarko force-pushed the kie-issues#1162-filter-expression branch from 0d7eec4 to 5fbeabc Compare June 19, 2024 14:44
@jomarko jomarko force-pushed the kie-issues#1162-filter-expression branch 2 times, most recently from da7575c to 960b438 Compare June 20, 2024 11:58
danielzhe
danielzhe approved these changes Jun 25, 2024
View reviewed changes
Copy link
Contributor

@danielzhe danielzhe left a comment

Choose a reason for hiding this comment

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

Everything in this PR is going to change to an easier way to write and read tests (except the images), but the code you made is going to be reutilized inside the API. So it is better to merge now so I can reuse this code inside the API.

ljmotta
ljmotta reviewed Jun 26, 2024
View reviewed changes
Copy link
Contributor

@ljmotta ljmotta left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@jomarko Thanks for addressing the previous comments. I've made a few more inline. Also, looking at the screenshots, I don't see a case where we resize the the filter expression to accommodate the inner text. Resizing the in and the match would be ideal.

Just a heads up, I've unresolved a previous comment regarding the default Literal Expression.

...xed-expression-component/src/expressions/FilterExpression/FilterExpressionCollectionCell.tsx Outdated Show resolved Hide resolved
...es/boxed-expression-component/src/expressions/FilterExpression/FilterExpressionMatchCell.tsx Outdated Show resolved Hide resolved
packages/boxed-expression-component/stories/boxedExpressions/Filter/Filter.stories.tsx Outdated Show resolved Hide resolved
packages/boxed-expression-component/stories/getDefaultBoxedExpressionForStories.ts Outdated Show resolved Hide resolved
packages/boxed-expression-component/tests-e2e/__fixtures__/boxedExpression.ts Outdated Show resolved Hide resolved
packages/boxed-expression-component/tests-e2e/__fixtures__/boxedExpression.ts
}

public async copyFilter(from: Page | Locator = this.page) {
await this.page.getByTestId("logic-type-button-test-id").click();
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess @danielzhe will change this with the new API?

packages/boxed-expression-component/tests-e2e/features/selection/selection.spec.ts Outdated Show resolved Hide resolved
ljmotta
ljmotta reviewed Jun 26, 2024
View reviewed changes
.../__screenshots__/Google-Chrome/features/resizing/boxed-filter-nested-resized-using-match.png Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

@danielzhe It looks like a bug on the auto resize due to the [ ]? 👀

ljmotta
ljmotta requested changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@ljmotta ljmotta left a comment

Choose a reason for hiding this comment

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

@jomarko Thanks for addressing all changes promptly. I've just saw one thing that slipped through my last review.

packages/boxed-expression-component/tests-e2e/boxedExpressions/filter/filterExpression.spec.ts Outdated
test("should correctly reset a nested filter", async ({ boxedExpressionEditor, page, stories }) => {
await stories.openBoxedFilter("nested");

await boxedExpressionEditor.resetFilter(page.locator("[data-ouia-component-id=expression-row-0]"));
Copy link
Contributor

Choose a reason for hiding this comment

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

We shouldn't be using the data-ouia-component-id.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thank you for the feedback, changes pushed

@tiagobento tiagobento merged commit b798710 into apache:main Jun 27, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljmotta ljmotta ljmotta approved these changes

@danielzhe danielzhe danielzhe approved these changes

@tiagobento tiagobento tiagobento approved these changes

@porcelli porcelli porcelli approved these changes

@ederign ederign Awaiting requested review from ederign

@paulovmr paulovmr Awaiting requested review from paulovmr

Assignees
No one assigned
Labels
area:dmn
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add end-to-end tests for Filter Expression on the Boxed Expression Component
5 participants
@jomarko @porcelli @tiagobento @danielzhe @ljmotta