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

Use Thread-Safe Dir.chdir Invocation in Tests #52596

Closed
wants to merge 1 commit into from

Conversation

Hamms
Copy link
Contributor

@Hamms Hamms commented Jul 4, 2023

Follow-up to #50743, which updated us to Ruby 3.0.

Ruby starting in 3.0 is much more proactive about preventing you from attempting to invoke Dir.chdir in a thread-unsafe way: https://bugs.ruby-lang.org/issues/15661

Unfortunately, we currently do just that in our Eyes/UI tests; each of the two individual rake tasks internally invokes Dir.chdir, and we execute the tasks in parallel. Fortunately, there's a simple fix: https://coderscat.com/ruby-change-current-working-directory/

Testing story

Tested locally that without this change, running bundle exec rake test:ui_all fails on Ruby 3.0 with conflicting chdir during another chdir block. With this change, we are able to successfully invoke both test tasks!

Ruby starting in 3.0 is much more proactive about preventing you from attempting to invoke `Dir.chdir` in a thread-unsafe way: https://bugs.ruby-lang.org/issues/15661

Unfortunately, we currently do just that in our Eyes/UI tests; each of the two individual rake tasks internally invokes `Dir.chdir`, and we execute the tasks in parallel. Fortunately, there's a simple fix: https://coderscat.com/ruby-change-current-working-directory/
@Hamms Hamms requested a review from a team July 4, 2023 00:25
@Hamms
Copy link
Contributor Author

Hamms commented Jul 4, 2023

nevermind, this is a locking solution which means we won't actually get the benefits of running tests in parallel this way

@Hamms Hamms closed this Jul 4, 2023
@Hamms Hamms deleted the thread-safe-dir-chdir branch July 4, 2023 00:39
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.

None yet

1 participant