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

fix: Improve the robustness of workspace SVG tests. #8689

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

BenHenning
Copy link
Contributor

@BenHenning BenHenning commented Dec 6, 2024

The basics

The details

Resolves

Fixes #8676

Proposed Changes

This largely reduces the dependence on direct value testing and instead updates clean-up tests to verify logic based on relative positioning. This guards against potential system-specific flakes that can occur when there are subtle calculation differences on different systems.

Note that the approach for improving robustness of the specific tests that were failing also provided an opportunity to clean up all of the other workspace SVG tests to make use of the same or similar helpers. I'm hoping that the latest versions are much more readable than they were previously.

Reason for Changes

For unknown reasons, different systems (such as OSX and specific versions of Ubuntu) would lead to differences in calculations for workspace tests. The tests really only needed to verify relative positioning between blocks, so this PR updates them to do exactly that (rather than verifying a block is at a specific relative position). This does introduce a bit more logic into the test, but it's managed by isolating this logic to helpers that help to self-document them and make it easier to verify the correctness via review.

Test Coverage

This PR is only changing tests. Technically the new helpers would benefit from direct tests, but this doesn't seem necessary given that they are relatively simple (and thus easy to verify).

Documentation

No documentation needs to be updated.

Additional Information

I found a flake in different tests while developing this and cataloged my findings in #8688.

This largely reduces the dependence on direct value testing and instead
updates clean-up tests to verify logic based on relative positioning.
This guards against potential system-specific flakes that can occur when
there are subtle calculation differences on different systems.
Copy link

conventional-commit-lint-gcf bot commented Dec 6, 2024

🤖 I detect that the PR title and the commit message differ and there's only one commit. To use the PR title for the commit history, you can use Github's automerge feature with squashing, or use automerge label. Good luck human!

-- conventional-commit-lint bot
https://conventionalcommits.org/

@github-actions github-actions bot added the PR: fix Fixes a bug label Dec 6, 2024
Copy link
Contributor Author

@BenHenning BenHenning left a comment

Choose a reason for hiding this comment

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

Self-reviewed changes.

@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Dec 6, 2024
@BenHenning BenHenning changed the title fix: Improve the robustness of workspace SVG tests and reduce flakes fix: Improve the robustness of workspace SVG tests. Dec 6, 2024
@github-actions github-actions bot added PR: fix Fixes a bug and removed PR: fix Fixes a bug labels Dec 6, 2024
@BenHenning
Copy link
Contributor Author

Note that the Ubuntu 22.x failure is one of the tests documented in #8688.

@BenHenning
Copy link
Contributor Author

NB: I'm ignoring the conventional message failure for now since I'll likely end up merging develop into this branch (and thus avoid the thing it's nitpicking on with the single commit scenario).

@BenHenning BenHenning marked this pull request as ready for review December 6, 2024 00:51
@BenHenning BenHenning requested a review from a team as a code owner December 6, 2024 00:51
@BenHenning
Copy link
Contributor Author

PTAL @cpcallen as well. Could you maybe run the tests locally since you were able to consistently trigger the failure before (and I wasn't)? It would be nice to see that it's also working for you.

@cpcallen cpcallen requested review from cpcallen and removed request for rachel-fenichel December 12, 2024 17:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: fix Fixes a bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failures due to #8550
3 participants