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.

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

e2e tests for useOverflow logic #2151

Draft
wants to merge 33 commits into
base: main
Choose a base branch
from
Draft

e2e tests for useOverflow logic #2151

wants to merge 33 commits into from

Conversation

r100-stack
Copy link
Member

@r100-stack r100-stack commented Jul 18, 2024

Changes

Pulled out of #2120 as PR 1 in the PR stack (PR stack order).

To make that PR easy to review, according to @mayank99's suggestion, I created this PR to only add new e2e tests that test the existing useOverflow logic. These tests will thus act as a baseline that the new useOverflow logic will also have to pass to make sure that there is no change in the resulting behavior.

Some e2e tests (e.g. MiddleTextTrucation) seem to be failing only on GitHub (e.g. action run) but not locally. I guess this is a small testing environment specific issue that I am currently trying to fix. Thus, the PR is ready for review despite the failing tests.

TODO:

  • Fix the flaky/unreliable e2e tests.

Testing

  • Ensured that the new e2e tests are passing.

Docs

N/A

After PR TODOs

  • Consider a lint rule for awaiting the expects in e2e tests (#2151 (comment))
  • Don't rely on implementation details when it's possible to pass props to the tag container/tags in ComboBox (#2151 (comment)) and Select (#2151 (comment)).

@r100-stack r100-stack self-assigned this Jul 18, 2024
@r100-stack r100-stack marked this pull request as ready for review July 18, 2024 19:14
@r100-stack r100-stack requested review from a team as code owners July 18, 2024 19:14
@r100-stack r100-stack requested review from mayank99 and Ben-Pusey-Bentley and removed request for a team July 18, 2024 19:14
Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

Are you also planning to delete the existing unit tests? Because otherwise it begs the question: why even add the e2e tests?

@r100-stack
Copy link
Member Author

Are you also planning to delete the existing unit tests?

It would be hard to know which unit tests would fail in the future with the new useOverflow. So, I thought to leave all in as of now. Then when introducing the new useOverflow, I plan to remove only the unit tests that are failing but have an e2e test equivalent that passes.

Because otherwise it begs the question: why even add the e2e tests?

If I create new e2e tests in the PR that adds the new useOverflow, it's hard to know whether there was any user facing change because we don't know if the e2e tests used to pass with the old useOverflow.

Thus, by adding those tests here with the old useOverflow, they can be baseline tests that the new useOverflow has to pass since it definitely used to pass with the old useOverflow.

Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

Some minor comments to improve tests and reduce diff.

Approving on the assumption that you can make CI pass.

testing/e2e/app/routes/Breadcrumbs/spec.ts Outdated Show resolved Hide resolved
testing/e2e/app/routes/Breadcrumbs/spec.ts Outdated Show resolved Hide resolved
testing/e2e/app/routes/Breadcrumbs/spec.ts Outdated Show resolved Hide resolved
testing/e2e/app/routes/ButtonGroup/route.tsx Outdated Show resolved Hide resolved
testing/e2e/app/routes/ButtonGroup/route.tsx Outdated Show resolved Hide resolved
testing/e2e/app/routes/ComboBox/spec.ts Outdated Show resolved Hide resolved
testing/e2e/app/routes/MiddleTextTruncation/spec.ts Outdated Show resolved Hide resolved
testing/e2e/app/routes/Select/route.tsx Outdated Show resolved Hide resolved
);
await expect(items.first()).toHaveAttribute('data-iui-focused', 'true');
const tags = await page
.locator('#test-component-selected-live > span')
Copy link
Contributor

Choose a reason for hiding this comment

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

this locator is relying on an implementation detail which could change at any time?

Copy link
Member Author

Choose a reason for hiding this comment

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

I was contemplating on whether to have selectors that depend on iTwinUI class names. That was why I used that span selector.

Changed to directly target -select-tag (057a5a0).

Copy link
Contributor

Choose a reason for hiding this comment

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

The select-tag is still coming from an internal class which can change at any time.

Right now we don't have a way to pass classes/attributes to the tag container or the tags (#894). This test could probably be rewritten to avoid relying on such implementation details (e.g. by only comparing the text inside the main container), but I'll leave it upto you if you want to rewrite it or add a TODO to wait until we allow customizing the tag container.

expectedLastTagTextContent: string | undefined;
}) => {
const tags = await page
.locator('div[class$="-select-tag-container"] > span')
Copy link
Contributor

Choose a reason for hiding this comment

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

this locator is also relying on an implementation detail?

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to directly target -select-tag (057a5a0).

Copy link
Contributor

Choose a reason for hiding this comment

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

Same as #2151 (comment), this is still relying on an implementation detail.

Copy link
Contributor

@Ben-Pusey-Bentley Ben-Pusey-Bentley left a comment

Choose a reason for hiding this comment

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

Approving so that this isn't held up. Added some comments that I hope will help with the failing e2e tests on the CI.

testing/e2e/app/routes/Breadcrumbs/spec.ts Outdated Show resolved Hide resolved
testing/e2e/app/routes/MiddleTextTruncation/spec.ts Outdated Show resolved Hide resolved
@r100-stack r100-stack marked this pull request as draft July 19, 2024 15:20
@r100-stack
Copy link
Member Author

Converting the PR to a draft to prevent it from merging before the other PRs in the PR stack are done.

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.

3 participants