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

Projects
None yet
3 participants
@gmcgibbon
Member

gmcgibbon commented Nov 8, 2018

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 馃槃

@rails-bot rails-bot bot added the activesupport label Nov 8, 2018

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:test_support_windows branch from 2642735 to 4dfff5a Nov 8, 2018

@@ -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/

This comment has been minimized.

@ahorek

ahorek Nov 8, 2018

Suggested change Beta
! 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/

This comment has been minimized.

@ahorek

ahorek Nov 8, 2018

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)

This comment has been minimized.

@gmcgibbon

gmcgibbon Nov 8, 2018

Member

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!

This comment has been minimized.

@eileencodes

eileencodes Nov 8, 2018

Member

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

This comment has been minimized.

@gmcgibbon

gmcgibbon Nov 8, 2018

Member

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.

This comment has been minimized.

@gmcgibbon

gmcgibbon Nov 9, 2018

Member

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?

This comment has been minimized.

@eileencodes

eileencodes Nov 9, 2018

Member

Does eager loading fix the circular autoload errors?

This comment has been minimized.

@gmcgibbon

gmcgibbon Nov 9, 2018

Member

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?

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:test_support_windows branch from 4dfff5a to 94f61de Nov 8, 2018

Windows support for parallelization and instrumenter
Add Windows support for `ActiveSupport::Testing::Parallelization`
and `ActiveSupport::Notifications::Instrumenter`.

@gmcgibbon gmcgibbon force-pushed the gmcgibbon:test_support_windows branch from 94f61de to 0712dfd Nov 8, 2018

@rails-bot rails-bot bot added the railties label Nov 8, 2018

@eileencodes eileencodes merged commit 9893998 into rails:master Nov 19, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gmcgibbon gmcgibbon deleted the gmcgibbon:test_support_windows branch Nov 19, 2018

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Dec 5, 2018

Add private `ActiveSupport::ProcessTime` module
In the codebase, we use `Process::CLOCK_PROCESS_CPUTIME_ID`,
but it doesn't work on Windows so we should add conditions
to support Windows. It is a good idea to handle all those cases
in one place to prevent spreading those conditions in the codebase.

Related to rails#34374, rails#34410

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Dec 5, 2018

Add private `ActiveSupport::ProcessClock` module
In the codebase, we use `Process::CLOCK_PROCESS_CPUTIME_ID`,
but it doesn't work on Windows so we should add conditions
to support Windows. It is a good idea to handle all those cases
in one place to prevent spreading those conditions in the codebase.

Related to rails#34374, rails#34410

bogdanvlviv added a commit to bogdanvlviv/rails that referenced this pull request Dec 5, 2018

Add private `ActiveSupport::ProcessClock` module
In the codebase, we use `Process::CLOCK_PROCESS_CPUTIME_ID`,
but it doesn't work on Windows so we should add conditions
to support Windows. It is a good idea to handle all those cases
in one place to prevent spreading those conditions in the codebase.

Related to rails#34374, rails#34410
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment