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

Test Fixes #25276

Merged
merged 4 commits into from
Feb 7, 2024
Merged

Test Fixes #25276

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1af9b8b
fixes masked-input component throwing error when there is no value
zofskeez Feb 7, 2024
5dc9ac0
removes copy button assertions from masked input test
zofskeez Feb 7, 2024
5f9bc62
reverts masked input assertion removal and updates test selectors
zofskeez Feb 7, 2024
f83b9cc
removes masked input assertion again
zofskeez Feb 7, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 4 additions & 3 deletions ui/lib/core/addon/components/masked-input.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,9 @@ export default class MaskedInputComponent extends Component {

get copyValue() {
// Value must be a string to be copied
if (typeof this.args.value === 'string') return this.args.value;
if (typeof this.args.value === 'object') return JSON.stringify(this.args.value);
return this.args.value.toString();
const { value } = this.args;
if (!value || typeof value === 'string') return value;
if (typeof value === 'object') return JSON.stringify(value);
return value.toString();
}
}
4 changes: 0 additions & 4 deletions ui/tests/integration/components/masked-input-test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,6 @@ module('Integration | Component | masked input', function (hooks) {
this.set('value', { some: 'object' });
await render(hbs`<MaskedInput @allowCopy={{true}} @value={{this.value}} />`);
assert.ok(component.copyButtonIsPresent);
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');
Comment on lines -52 to -55
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 •
edited
Loading

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

});

test('it renders a download button when allowDownload is true', async function (assert) {
Expand Down
Loading