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

Misc facet playwright #2484

Merged
merged 13 commits into from
Jul 4, 2024
Merged

Misc facet playwright #2484

merged 13 commits into from
Jul 4, 2024

Conversation

jrchudy
Copy link
Member

@jrchudy jrchudy commented Jun 27, 2024

In this PR, the misc-facet spec is migrated to playwright. With this, all of the delete prohibited specs are also migrated so that protractor configuration is removed as well. I also removed the command in the makefile and updated e2e.yml.

Other changes in this PR:

  • add chance declaration for generating random content in an input field
  • moved a check in histogram tests to validate button is disabled before checking the input values
    • button is no longer disabled when we know the input values have updated after zooming in
  • classname of a spinner in recordset changed

@jrchudy jrchudy requested a review from RFSH July 2, 2024 00:40
Copy link
Member

@RFSH RFSH left a comment

Choose a reason for hiding this comment

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

I like the changes that you've done and love how fast the playwright facet spec is!

I just have some minor suggestions before pushing the changes:

  • While I didn't see any issues related to this, but to make sure it doesn't cause issues at some point in the future, you should make sure the modal is closed after clicking on the close button. So basically when you're doing getCloseBtn().click, the next line should be: await expect.soft(modal).not.toBeAttached();. We could have a helper function for this too. The function would do the click and wait for the modal to disappear. It's up to you if a function is needed or not.

  • Like I mentioned in my inline comment, I think your new functions need jsdoc so it's easier to know what they're doing.

  • I think we should rename ind-facet.config.ts to something like main-facet.config.ts or just facet.config.ts. It's a bit confusing that it's using the same name as one of the specs, but it includes more than just that one spec.

test/e2e/utils/recordset-utils.ts Outdated Show resolved Hide resolved
@jrchudy jrchudy merged commit 2e6b09e into master Jul 4, 2024
1 check passed
@jrchudy jrchudy deleted the misc-facet-playwright branch July 4, 2024 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants