serpapi / turbo_tests

Run RSpec tests on multiple cores. Like parallel_tests but with incremental summarized output. Originally extracted from the Discourse and Rubygems source code.
https://rubygems.org/gems/turbo_tests
MIT License
172 stars 26 forks source link

When a exception is raised out of specs or code, tests are marked as successful #5

Closed kadru closed 3 years ago

kadru commented 3 years ago

For example you can raise an exception in spec/rails_helper.rb with raise 'foo', eg:

# This file is copied to spec/ when you run 'rails generate rspec:install'
require 'spec_helper'
ENV['RAILS_ENV'] ||= 'test'
require File.expand_path('../config/environment', __dir__)
# Prevent database truncation if the environment is production
abort("The Rails environment is running in production mode!") if Rails.env.production?
require 'rspec/rails'

# RAISE ERROR
raise "foo"

begin
  ActiveRecord::Migration.maintain_test_schema!
rescue ActiveRecord::PendingMigrationError => e
  puts e.to_s.strip
  exit 1
end
RSpec.configure do |config|

  config.fixture_path = "#{::Rails.root}/spec/fixtures"
  config.use_transactional_fixtures = true

  config.infer_spec_type_from_file_location!

  config.filter_rails_from_backtraceend
end

Then I run bundle exec turbo_tests And the output wil be:

8 processes for 8 specs, ~ 1 specs per process

Finished in 1.47 seconds (files took 0 seconds to load)
0 examples, 0 failures

I expect the tests to fail, just like bundle exec rspec would fail:

An error occurred while loading ./spec/helpers/widgets_helper_spec.rb.
Failure/Error: raise "foo"

RuntimeError:
  foo
# ./spec/rails_helper.rb:25:in `<top (required)>'
# ./spec/helpers/widgets_helper_spec.rb:1:in `require'
# ./spec/helpers/widgets_helper_spec.rb:1:in `<top (required)>'
...

I'm using rails 6.0.3.4, rspec-rails 4.0.2 and turbo_tests 1.2.

ilyazub commented 3 years ago

Thanks for reporting a bug! We also encountered it today. I'll try to come up with a fix.

grk commented 3 years ago

The exit status of the subprocesses is ignored in https://github.com/serpapi/turbo_tests/blob/master/lib/turbo_tests/runner.rb#L121 - you can check it with wait_thr.value, but this will block until all processes return.

Something like this:

@@ -48,7 +48,7 @@ module TurboTests
           :runtime_log => @runtime_log
         )

-      tests_in_groups.each_with_index do |tests, process_id|
+      wait_threads = tests_in_groups.each_with_index.map do |tests, process_id|
         start_regular_subprocess(tests, process_id + 1)
       end

@@ -58,7 +58,7 @@ module TurboTests

       @threads.each(&:join)

-      @reporter.failed_examples.empty?
+      wait_threads.map(&:value).all?(&:success?) && @reporter.failed_examples.empty?
     end

   protected
@@ -106,7 +106,7 @@ module TurboTests

         puts "TEST_ENV_NUMBER=#{env["TEST_ENV_NUMBER"]} #{rerun_command.join(" ")}"

-        _stdin, stdout, stderr, _wait_thr = Open3.popen3(env, *command)
+        _stdin, stdout, stderr, wait_thr = Open3.popen3(env, *command)

         @threads <<
           Thread.new do
@@ -129,6 +129,8 @@ module TurboTests
           end

         @threads << start_copy_thread(stderr, STDERR)
+
+        wait_thr
       end
     end

would at least make the whole process return 1, so you'd get a failed run on CI instead of current silent failure.

ilyazub commented 3 years ago

@grk Thank you for looking into this and specifically for suggesting the possible fix! It's definitely a good starting point. I'll test the diff you've posted.