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

Fix a number of flaky tests (.ordered expectation in multiple threads, and unstable output expectations) #736

Merged
merged 4 commits into from Dec 18, 2019
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 5 additions & 1 deletion .travis.yml
Expand Up @@ -11,7 +11,11 @@ rvm:
branches:
only: [master]
sudo: false
cache: bundler
cache:
bundler: true
directories:
- spec/fixtures/rails51/vendor/bundle
- spec/fixtures/rails52/vendor/bundle
matrix:
allow_failures:
- rvm: ruby-head
Expand Down
52 changes: 25 additions & 27 deletions spec/integration_spec.rb
Expand Up @@ -160,15 +160,13 @@ def test_unicode
it "can show simulated output when serializing stdout" do
write 'spec/xxx_spec.rb', 'describe("it"){it("should"){sleep 0.5; puts "TEST1"}}'
write 'spec/xxx2_spec.rb', 'describe("it"){it("should"){sleep 1; puts "TEST2"}}'
result = run_tests "spec", :type => 'rspec', :add => "--serialize-stdout", export: {'PARALLEL_TEST_HEARTBEAT_INTERVAL' => '0.1'}

result = run_tests "spec", :type => 'rspec', :add => "--serialize-stdout", export: {'PARALLEL_TEST_HEARTBEAT_INTERVAL' => '0.01'}
expect(result).to match(/\.{4}.*TEST1.*\.{4}.*TEST2/m)
end

it "can show simulated output preceded by command when serializing stdout with verbose option" do
write 'spec/xxx_spec.rb', 'describe("it"){it("should"){sleep 1; puts "TEST1"}}'
result = run_tests "spec --verbose", :type => 'rspec', :add => "--serialize-stdout", export: {'PARALLEL_TEST_HEARTBEAT_INTERVAL' => '0.2'}

result = run_tests "spec --verbose", :type => 'rspec', :add => "--serialize-stdout", export: {'PARALLEL_TEST_HEARTBEAT_INTERVAL' => '0.02'}
expect(result).to match(/\.{5}.*\nbundle exec rspec spec\/xxx_spec\.rb\nTEST1/m)
end

Expand Down Expand Up @@ -229,7 +227,7 @@ def test_unicode

it "runs with --group-by found" do
# it only tests that it does not blow up, as it did before fixing...
write "spec/x1_spec.rb", "puts '111'"
write "spec/x1_spec.rb", "puts 'TEST111'"
run_tests "spec", :type => 'rspec', :add => '--group-by found'
end

Expand All @@ -254,27 +252,27 @@ def test_unicode
end

it "runs with files that have spaces" do
write "test/xxx _test.rb", 'puts "YES"'
write "test/xxx _test.rb", 'puts "TEST_SUCCESS"'
result = run_tests("test", processes: 2, type: 'test')
expect(result).to include "YES"
expect(result).to include "TEST_SUCCESS"
end

it "uses relative paths for easy copying" do
write "test/xxx_test.rb", 'puts "YES"'
write "test/xxx_test.rb", 'puts "Test output: YES"'
result = run_tests("test", processes: 2, type: 'test', add: '--verbose')
expect(result).to include "YES"
expect(result).to include "Test output: YES"
expect(result).to include "[test/xxx_test.rb]"
expect(result).not_to include Dir.pwd
end

it "can run with given files" do
write "spec/x1_spec.rb", "puts '111'"
write "spec/x2_spec.rb", "puts '222'"
write "spec/x3_spec.rb", "puts '333'"
write "spec/x1_spec.rb", "puts 'TEST111'"
write "spec/x2_spec.rb", "puts 'TEST222'"
write "spec/x3_spec.rb", "puts 'TEST333'"
result = run_tests "spec/x1_spec.rb spec/x3_spec.rb", :type => 'rspec'
expect(result).to include('111')
expect(result).to include('333')
expect(result).not_to include('222')
expect(result).to include('TEST111')
expect(result).to include('TEST333')
expect(result).not_to include('TEST222')
end

it "can run with test-options" do
Expand All @@ -297,23 +295,23 @@ def test_unicode
end

it "filters test by given pattern and relative paths" do
write "spec/x_spec.rb", "puts 'XXX'"
write "spec/y_spec.rb", "puts 'YYY'"
write "spec/z_spec.rb", "puts 'ZZZ'"
write "spec/x_spec.rb", "puts 'TESTXXX'"
write "spec/y_spec.rb", "puts 'TESTYYY'"
write "spec/z_spec.rb", "puts 'TESTZZZ'"
result = run_tests "spec", :add => "-p '^spec/(x|z)'", :type => "rspec"
expect(result).to include('XXX')
expect(result).not_to include('YYY')
expect(result).to include('ZZZ')
expect(result).to include('TESTXXX')
expect(result).not_to include('TESTYYY')
expect(result).to include('TESTZZZ')
end

it "excludes test by given pattern and relative paths" do
write "spec/x_spec.rb", "puts 'XXX'"
write "spec/acceptance/y_spec.rb", "puts 'YYY'"
write "spec/integration/z_spec.rb", "puts 'ZZZ'"
write "spec/x_spec.rb", "puts 'TESTXXX'"
write "spec/acceptance/y_spec.rb", "puts 'TESTYYY'"
write "spec/integration/z_spec.rb", "puts 'TESTZZZ'"
result = run_tests "spec", :add => "--exclude-pattern 'spec/(integration|acceptance)'", :type => "rspec"
expect(result).to include('XXX')
expect(result).not_to include('YYY')
expect(result).not_to include('ZZZ')
expect(result).to include('TESTXXX')
expect(result).not_to include('TESTYYY')
expect(result).not_to include('TESTZZZ')
end

it "can wait_for_other_processes_to_finish" do
Expand Down
4 changes: 2 additions & 2 deletions spec/parallel_tests/cli_spec.rb
Expand Up @@ -315,8 +315,8 @@ def self.it_prints_nothing_about_rerun_commands(options)
it "run twice with multiple groups" do
skip "fails on jruby" if RUBY_PLATFORM == "java"
options = common_options.merge(count: 3, only_group: [2,3])
expect(subject).to receive(:run_tests).once.ordered.with(['ccc', 'ddd'], 0, 1, options).and_return(results)
expect(subject).to receive(:run_tests).once.ordered.with(['eee', 'fff'], 1, 1, options).and_return(results)
expect(subject).to receive(:run_tests).once.with(['ccc', 'ddd'], 0, 1, options).and_return(results)
expect(subject).to receive(:run_tests).once.with(['eee', 'fff'], 1, 1, options).and_return(results)
subject.run(['test', '-n', '3', '--only-group', '2,3', '-t', 'my_test_runner'])
end
end
Expand Down
2 changes: 0 additions & 2 deletions spec/rails_spec.rb
@@ -1,8 +1,6 @@
require 'spec_helper'

describe 'rails' do
let(:test_timeout) { 420 } # this can take very long on fresh bundle ...

def sh(command, options={})
result = ''
IO.popen(options.fetch(:environment, {}), command, err: [:child, :out]) do |io|
Expand Down
6 changes: 0 additions & 6 deletions spec/spec_helper.rb
Expand Up @@ -188,12 +188,6 @@ def setup_runtime_log

config.raise_errors_for_deprecations!

# sometimes stuff hangs -> do not hang everything
config.include(Module.new {def test_timeout;30;end })
Copy link
Owner

Choose a reason for hiding this comment

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

still need this though ?
... having tests just stop and hang is hard to debug :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure I can re-add that! But I think the Timeout block was actually swallowing some errors and making it super hard to figure out this test failure, and when I removed it the test started failing consistently, so it was easy to fix. So my theory is that those regular timeout errors might have actually been caused by this ordered expectation.

But I'll run my delayed commit script again for a while and create 15 more builds, and will check to see if there's any other flaky tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

I remember when I last worked on stabilizing CI here, I also temporarily removed this because it seemed to be indeed swallowing errors somehow. 👍

Copy link
Owner

Choose a reason for hiding this comment

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

bringing them back in #741 with a dedicated class and note on debugging

config.around do |example|
Timeout.timeout(test_timeout, &example)
end

config.after do
ENV.delete "PARALLEL_TEST_GROUPS"
ENV.delete "PARALLEL_TEST_PROCESSORS"
Expand Down