-
Notifications
You must be signed in to change notification settings - Fork 227
Improve go to relevant file performance #3731
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
Improve go to relevant file performance #3731
Conversation
How to use the Graphite Merge QueueAdd the label graphite-merge to this PR to add it to the merge queue. You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
jenny-codes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the idea but left some comments!
3041894 to
5d0e220
Compare
|
Thanks for the feedback @jenny-codes! I corrected all revisions, If you have any more feedback let me know. Also checked again manually on the rubocop, rails and Buk monorepo to be sure it works well 👍🏻 . |
5d0e220 to
ed632b0
Compare
|
I changed to the relative path implementation. It works fine with manual testing. I will try to change the test to use a fixture folder in the filesystem instead of stubbing the responses to test the implementation without harcoding it. |
ddf2ae5 to
fc31e36
Compare
|
@jenny-codes I ended up testing the feature using a temporary directory strategy. That way the stubs are not implementation specific, the test is autocontained, and we don't need a real fixture folder. |
fc31e36 to
a30f4dd
Compare
jenny-codes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the update! The implementation looks good but I wonder if it'd be worth it to add a test that verifies that we will search only within the "search root". We can make a test where we have this file structure
root
- a
- test
- implementation_test.rb
- implementation.rb
- b
- test
- implementation_test.rb
- implementation.rb
and assert that we don't return the implementation <> test results for a in b.
By doing a walk over the tree of directories in the workpsace we improve the dir Glob time because it only checks for the closest test directory folder, instead of searching in all the files of the workspace.
a30f4dd to
81658a2
Compare
|
@jenny-codes, changed the test to only create the files, and added the "test within root" test to check that we don't get tests outside the search root. |
jenny-codes
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Thank you!
|
@jenny-codes I have no permissions to merge to shopify upstream, just pinging in case you forgot, no hurry on this PR. |
|
@domingo2000 thank you! |
|
Thank to you!, I really use this feature a lot, and have used a lot your initial implementation. |
By doing a walk over the tree of directories in the workpsace we improve the dir Glob time because it only checks for the closest test directory folder, instead of searching in all the files of the workspace.
Motivation
Closes #3327
Implementation
As discussed here the implementation consist in a tree walk over the directory structure. By doing this walk until the workspace path, we only check for test files in the most closer ancestor folder that contains tests, the locality of tests in projects using engines in rails, or Packwerck, or in general in ruby projects, make the search correct most of the times.
By checking fewer directories the performance of the Glob that search for files is improved drastically, making the go to relevant file feels instant in big projects.
Automated Tests
Changed the current test implementation to use tempdirs and create some examples in that tempdir. That way the tests are not implementation specific, as it was when stubbing the filesystem calls.
Manual Tests
screenrecording-2025-08-30_16-11-24.mp4
screenrecording-2025-08-30_16-15-06.mp4
screenrecording-2025-08-30_16-14-19.mp4