Skip to content

Conversation

@Dhaulagiri
Copy link
Collaborator

@Dhaulagiri Dhaulagiri commented Jun 16, 2022

📌 Summary

Fixes #358

Refactors a single test to avoid an odd bug in our test suite

🛠️ Detailed description

After some sleuthing 🕵️ I found out how to reproduce the issue (in CI, never locally 🤷 )

I'm a bit rusty, but I made a pass at refactoring this test to not use sinon and a) the test "works" and b) the errors are no longer present in our CI runs if you look at one of the tests for components.

Ideally we could understand why this test was causing this in the event we want to add sinon back in the future but I've hit my timebox limit on this for now. My suspicion is that the sinon usage was causing some hidden error that sent things astray.


👀 How to review

👉 Review commit-by-commit

Reviewer's checklist:

  • +1 Percy if applicable
  • Confirm that PR has a changelog update via Changesets if needed

💬 Please consider using conventional comments when reviewing this PR.

@changeset-bot
Copy link

changeset-bot bot commented Jun 16, 2022

⚠️ No Changeset found

Latest commit: 3f68ce5

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@vercel
Copy link

vercel bot commented Jun 16, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
hds-components ✅ Ready (Inspect) Visit Preview Jun 16, 2022 at 6:13PM (UTC)
hds-flight-website ✅ Ready (Inspect) Visit Preview Jun 16, 2022 at 6:13PM (UTC)

@vercel vercel bot temporarily deployed to Preview – hds-flight-website June 16, 2022 13:31 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components June 16, 2022 13:31 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website June 16, 2022 13:33 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components June 16, 2022 13:33 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website June 16, 2022 14:00 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components June 16, 2022 14:00 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components June 16, 2022 16:47 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website June 16, 2022 16:47 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website June 16, 2022 17:11 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components June 16, 2022 17:11 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website June 16, 2022 17:16 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components June 16, 2022 17:17 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website June 16, 2022 17:24 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components June 16, 2022 17:24 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website June 16, 2022 17:33 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components June 16, 2022 17:33 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website June 16, 2022 17:36 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components June 16, 2022 17:36 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website June 16, 2022 17:39 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components June 16, 2022 17:39 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website June 16, 2022 17:43 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components June 16, 2022 17:43 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website June 16, 2022 17:46 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components June 16, 2022 17:46 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website June 16, 2022 17:52 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components June 16, 2022 17:52 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website June 16, 2022 18:07 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components June 16, 2022 18:07 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-flight-website June 16, 2022 18:13 Inactive
@vercel vercel bot temporarily deployed to Preview – hds-components June 16, 2022 18:13 Inactive
@Dhaulagiri Dhaulagiri changed the title [wip] percy investiation Fix odd percy bugy Jun 16, 2022
@Dhaulagiri Dhaulagiri changed the title Fix odd percy bugy Fix odd percy bug Jun 16, 2022
@Dhaulagiri Dhaulagiri changed the title Fix odd percy bug Fix odd test bug Jun 16, 2022
@Dhaulagiri Dhaulagiri marked this pull request as ready for review June 16, 2022 18:21
@Dhaulagiri Dhaulagiri requested a review from alex-ju June 16, 2022 18:22
Copy link
Member

@alex-ju alex-ju left a comment

Choose a reason for hiding this comment

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

Thanks for investigating this! I've also spent some time yesterday trying to replicate the issue locally but no luck.

let clicked = false;
this.set('clickHandler', () => (clicked = true));
await render(
hbs`<div {{on "click" this.clickHandler}}><Hds::Interactive @href="javascript:;" id="test-interactive"/></div>`
Copy link
Member

Choose a reason for hiding this comment

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

So we rely on the click event bubbling up, then capture it at the parent level using a boolean. Given we only use sinon for this test I'm happy with the change. We may get back to square one if we won't manage to use this pattern for future scenarios, but that's a problem for tomorrow.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

We may get back to square one if we won't manage to use this pattern for future scenarios

That was my fear with "fixing" it this way as well.

Copy link
Contributor

@MelSumner MelSumner left a comment

Choose a reason for hiding this comment

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

:shipit:

@Dhaulagiri Dhaulagiri merged commit 87dbb08 into main Jun 17, 2022
@Dhaulagiri Dhaulagiri deleted the br-percy-test branch June 17, 2022 13:08
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.

percy duplicate snapshots error

3 participants