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

Test Fixes #25276

Merged
merged 4 commits into from
Feb 7, 2024
Merged

Test Fixes #25276

merged 4 commits into from
Feb 7, 2024

Conversation

zofskeez
Copy link
Contributor

@zofskeez zofskeez commented Feb 7, 2024

There was an issue where the test-ui GH action was being skipped but the check was reporting as passed which led to some PR's being merged that had failures and this PR fixes them.

@zofskeez zofskeez added this to the 1.16.0-rc1 milestone Feb 7, 2024
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Feb 7, 2024
Copy link

github-actions bot commented Feb 7, 2024

Build Results:
All builds succeeded! ✅

Copy link

github-actions bot commented Feb 7, 2024

CI Results:
All Go tests succeeded! ✅

@hellobontempo
Copy link
Contributor

Thank you for tackling these! PRs we think were merged with failures (noting for backport posterity)
#25235
#25269

Comment on lines -52 to -55
await click('[data-test-copy-icon]');
assert
.dom('flight-icon-clipboard-checked')
.exists('clicking copy icon copies value to clipboard', 'copy is successful when value is an object');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test was originally failing from an unfound selector and the selector should be [data-test-copy-button]. After updating however, the assertion failed since a different icon was displaying after the button was clicked. I paused the test and it was a red x icon. I'm guessing that the HDS copy button component had an issue copying to the clipboard in the test environment. In any case it feels like that functionality is not the concern of the masked input so I removed the click and assertion.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sometimes the copy button requires a container - the component still works in the app, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it does. It actually works in the test after it has been paused and you manually click it again too.

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the test was actually added recently, so I wonder if it's not copying the value as expected? https://github.com/hashicorp/vault/pull/25269/files

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I noticed on second glance that the flight-icon-clipboard-checked selector was missing the . since it was a class but even after updating that it just does not pass locally or on CI which must be a test environment issue since it is working in the app.

Copy link
Contributor Author

@zofskeez zofskeez Feb 7, 2024

Choose a reason for hiding this comment

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

Pausing the test seems to confirm this. The first click happens from within the test and you can see the icon changes to a failure state. Since the test is paused it's almost like the navigator API becomes available and when I click it again manually it works.

Screen.Recording.2024-02-07.at.3.31.58.PM.mov

@zofskeez zofskeez enabled auto-merge (squash) February 7, 2024 23:02
Copy link
Collaborator

@hashishaw hashishaw left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@zofskeez zofskeez merged commit bfe50ee into main Feb 7, 2024
31 checks passed
@zofskeez zofskeez deleted the ui/test-fixes branch February 7, 2024 23:24
Monkeychip pushed a commit that referenced this pull request Feb 12, 2024
* fixes masked-input component throwing error when there is no value

* removes copy button assertions from masked input test

* reverts masked input assertion removal and updates test selectors
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed pr/no-changelog ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants