Skip to content

chore: Don't render certain components when running meticulous tests#10772

Merged
shivamklr merged 17 commits intomainfrom
chore/add-meticulous-ignore
Aug 24, 2023
Merged

chore: Don't render certain components when running meticulous tests#10772
shivamklr merged 17 commits intomainfrom
chore/add-meticulous-ignore

Conversation

@keithwillcode
Copy link
Copy Markdown
Contributor

@keithwillcode keithwillcode commented Aug 15, 2023

What does this PR do?

Fixes the red X's we see when Meticulous runs and detects visual changes that are triggered by URL or version number changes, which are not real changes.

Type of change

  • Chore (refactoring code, technical debt, workflow improvements)

@vercel
Copy link
Copy Markdown

vercel Bot commented Aug 15, 2023

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

Name Status Preview Comments Updated (UTC)
api ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 23, 2023 1:15pm
cal-demo ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 23, 2023 1:15pm
dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 23, 2023 1:15pm
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
cal ⬜️ Ignored (Inspect) Visit Preview Aug 23, 2023 1:15pm
qa ⬜️ Ignored (Inspect) Visit Preview Aug 23, 2023 1:15pm
ui ⬜️ Ignored (Inspect) Visit Preview Aug 23, 2023 1:15pm

@github-actions
Copy link
Copy Markdown
Contributor

github-actions Bot commented Aug 15, 2023

Thank you for following the naming conventions! 🙏

@zomars zomars added the core area: core, team members only label Aug 15, 2023
@keithwillcode
Copy link
Copy Markdown
Contributor Author

keithwillcode commented Aug 15, 2023

@shivamklr Want to take this over and fix the places where we show a URL, which per deployment is different and causing meticulous to think there are differences? cc @emrysal

@keithwillcode keithwillcode changed the title chore: Don't render credits when running meticulous tests chore: Don't render certain components when running meticulous tests Aug 15, 2023
@shivamklr
Copy link
Copy Markdown
Contributor

@keithwillcode Taking a look.

Copy link
Copy Markdown
Contributor Author

@keithwillcode keithwillcode left a comment

Choose a reason for hiding this comment

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

@shivamklr Nice work. @emrysal and I chatted about this and think we should put something like const IS_VISUAL_REGRESSION_TESTING = Boolean(globalThis.window?.Meticulous?.isRunningAsTest); in constants.ts for the following reasons:

  • It's more obvious as to what the code is doing
  • It shields the dependency of using Meticulous behind a "wall" instead of being repeated multiple times throughout the code.

Copy link
Copy Markdown
Contributor Author

@keithwillcode keithwillcode left a comment

Choose a reason for hiding this comment

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

@Shivam I see sessions running on localhost. Can we update the code such that Meticulous isn't recording those.

@shivamklr shivamklr disabled auto-merge August 21, 2023 13:37
@shivamklr
Copy link
Copy Markdown
Contributor

This PR is not yet ready to merge. I am trying to figure out what would meticulous show in the dashboard after the following changes.

Copy link
Copy Markdown
Member

@sean-brydon sean-brydon left a comment

Choose a reason for hiding this comment

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

I'd honestly rather remove meticulous than have to manage this across the app. Let me know what you think.

@keithwillcode
Copy link
Copy Markdown
Contributor Author

I'd honestly rather remove meticulous than have to manage this across the app. Let me know what you think.

Yeah I see what you mean. Fortunately it seems to just be a few places we need this. If it were more, I’d definitely say it would be a harder sell.

I think a good plan is to get this running and passing, see the value we get and then decide from there.

Copy link
Copy Markdown
Member

@hariombalhara hariombalhara left a comment

Choose a reason for hiding this comment

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

Agree with both @sean-brydon and @keithwillcode

Let's see if benefits outweigh the drawbacks. The biggest benefit that I see is it being low cost in maintenance.

Also, which components in general we should not render?
Edit: I saw that in PR description

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automated-tests area: unit tests, e2e tests, playwright core area: core, team members only Medium priority Created by Linear-GitHub Sync

Projects

No open projects
Status: No status

Development

Successfully merging this pull request may close these issues.

5 participants