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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows support for parallelization and instrumenter #34410

Merged
merged 1 commit into from
Nov 19, 2018

Conversation

gmcgibbon
Copy link
Member

Summary

Fixes #34374.

Adds Windows support for ActiveSupport::Testing::Parallelization and ActiveSupport::Notifications::Instrumenter.

Unsure of how I can adequately test this aside from adding a Windows build to CI.

r? @eileencodes since you added test parallelization and event CPU time to Rails 馃槃

@@ -55,6 +55,11 @@ class Event
attr_reader :name, :time, :transaction_id, :payload, :children
attr_accessor :end

def self.clock_gettime_supported? # :nodoc:
defined?(Process::CLOCK_PROCESS_CPUTIME_ID) &&
! RUBY_PLATFORM =~ /bccwin|cygwin|emx|mingw|mswin|wince/
Copy link

Choose a reason for hiding this comment

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

Suggested change
! RUBY_PLATFORM =~ /bccwin|cygwin|emx|mingw|mswin|wince/
!Gem.win_platform?

@@ -1,7 +1,7 @@
# frozen_string_literal: true

require "drb"
require "drb/unix"
require "drb/unix" unless RUBY_PLATFORM =~ /bccwin|cygwin|emx|mingw|mswin|wince/
Copy link

Choose a reason for hiding this comment

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

doesn't work for me, you should skip it at
https://github.com/rails/rails/blob/master/activesupport/lib/active_support/test_case.rb#L14

'drb/unix' is not available
Traceback (most recent call last):
        22: from bin/test:5:in `<main>'
        21: from bin/test:5:in `require_relative'
        20: from C:/rails/tools/test.rb:26:in `<top (required)>'
        19: from C:/rails/railties/lib/rails/test_unit/runner.rb:39:in `run'
        18: from C:/rails/railties/lib/rails/test_unit/runner.rb:50:in `load_tests'
        17: from C:/rails/railties/lib/rails/test_unit/runner.rb:50:in `each'
        16: from C:/rails/railties/lib/rails/test_unit/runner.rb:50:in `block in load_tests'
        15: from C:/rails/railties/lib/rails/test_unit/runner.rb:50:in `require'
        14: from C:/rails/actionpack/test/abstract/callbacks_test.rb:3:in `<top (required)>'
        13: from C:/rails/actionpack/test/abstract/callbacks_test.rb:3:in `require'
        12: from C:/rails/actionpack/test/abstract_unit.rb:428:in `<top (required)>'
        11: from C:/rails/actionpack/test/abstract_unit.rb:428:in `new'
        10: from C:/rails/actionpack/test/abstract_unit.rb:385:in `initialize'
         9: from C:/Ruby25-x64/lib/ruby/2.5.0/drb/drb.rb:1710:in `start_service'
         8: from C:/Ruby25-x64/lib/ruby/2.5.0/drb/drb.rb:1710:in `new'
         7: from C:/Ruby25-x64/lib/ruby/2.5.0/drb/drb.rb:1404:in `initialize'
         6: from C:/Ruby25-x64/lib/ruby/2.5.0/drb/drb.rb:772:in `open_server'
         5: from C:/Ruby25-x64/lib/ruby/2.5.0/drb/drb.rb:804:in `auto_load'
         4: from C:/rails/activesupport/lib/active_support/dependencies.rb:297:in `require'
         3: from C:/rails/activesupport/lib/active_support/dependencies.rb:263:in `load_dependency'
         2: from C:/rails/activesupport/lib/active_support/dependencies.rb:297:in `block in require'
         1: from C:/rails/activesupport/lib/active_support/dependencies.rb:297:in `require'
C:/Ruby25-x64/lib/ruby/2.5.0/drb/unix.rb:6:in `<top (required)>': UNIXServer is required (LoadError)

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, I was able to run tests with a dummy app on Ruby 2.5.3 on Windows 10 using this code, but removing parallelization when on Windows is likely a cleaner solution if DRb isn't properly supported. I'll see what I can do!

Copy link
Member

Choose a reason for hiding this comment

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

We can flip it to use threads by default in Windows like we do for JRuby

Copy link
Member Author

Choose a reason for hiding this comment

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

Great idea! I assume CRuby on Windows won't get the same threading advantages as JRuby, but it should still work. I edited the test_helper template to use threads when on Windows.

Copy link
Member Author

Choose a reason for hiding this comment

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

This seems to be working on my setup with paralellize(workers: 2, with: :threads), but I get circular autoload errors when running parallelized tests. Do we want to recommend eager loading when parallelizing with threads?

Copy link
Member

Choose a reason for hiding this comment

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

Does eager loading fix the circular autoload errors?

Copy link
Member Author

@gmcgibbon gmcgibbon Nov 9, 2018

Choose a reason for hiding this comment

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

It does. I got one or two SQLite busy exceptions between a few test runs once I eager loaded but I think that's a separate threading issue. I talked to @rafaelfranca about the circular autoload errors and he mentioned @matthewd's work on https://github.com/rails/rails/blob/master/activesupport/lib/active_support/dependencies/interlock.rb. Maybe this can be avoided without eager load somehow?

Add Windows support for `ActiveSupport::Testing::Parallelization`
and `ActiveSupport::Notifications::Instrumenter`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants