Skip to content

Commit c93bde2

Browse files
authored
bugfix: Fix up problematic interceptor specs (JRuby issues on global state leak) (#1783)
* Dummy commit; * POC: Spike on jruby for cucumber 9.1.2 * Debug the response * Output response clean * Further debug each problematic method * Add note of problematic methods * Fix rubocop annoyance * Comment out problematic code * Add before / after for each other method * Simplify test * Update before/after hooks to be on both stdout / stderr * Tidy up interceptor spec * Remove dummy file * Add changelog entry * Update rubocop todo file based on new issues raised from changes to make JRuby pass
1 parent a21aea7 commit c93bde2

File tree

3 files changed

+154
-122
lines changed

3 files changed

+154
-122
lines changed

.rubocop_todo.yml

+18-9
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
# This configuration was generated by
22
# `rubocop --auto-gen-config`
3-
# on 2025-01-06 16:14:48 UTC using RuboCop version 1.69.2.
3+
# on 2025-03-14 09:41:32 UTC using RuboCop version 1.69.2.
44
# The point is for the user to remove these configuration records
55
# one by one as the offenses are removed from the code base.
66
# Note that changes in the inspected code, or installation of new
@@ -10,6 +10,7 @@
1010
# TODO - [LH] -> Jul '24 - 370 files inspected, 637 offenses detected, 97 offenses autocorrectable
1111
# TODO - [LH] -> Jul '24 - 369 files inspected, 661 offenses detected, 98 offenses autocorrectable
1212
# TODO - [LH] -> Jan '25 (Updated deps and v10 prep) - 369 files inspected, 704 offenses detected, 112 offenses autocorrectable
13+
# TODO - [LH] -> Mar '25 (v10 prep) - 370 files inspected, 721 offenses detected, 116 offenses autocorrectable
1314

1415
# Offense count: 1
1516
# This cop supports safe autocorrection (--autocorrect).
@@ -45,7 +46,7 @@ Lint/UselessMethodDefinition:
4546
Exclude:
4647
- 'lib/cucumber/glue/proto_world.rb'
4748

48-
# Offense count: 60
49+
# Offense count: 61
4950
# Configuration parameters: AllowedMethods, AllowedPatterns, CountRepeatedAttributes.
5051
Metrics/AbcSize:
5152
Max: 127
@@ -66,17 +67,17 @@ Metrics/ClassLength:
6667
Metrics/CyclomaticComplexity:
6768
Max: 12
6869

69-
# Offense count: 74
70+
# Offense count: 76
7071
# Configuration parameters: CountComments, CountAsOne, AllowedMethods, AllowedPatterns.
7172
Metrics/MethodLength:
7273
Max: 64
7374

74-
# Offense count: 16
75+
# Offense count: 15
7576
# Configuration parameters: CountComments, CountAsOne.
7677
Metrics/ModuleLength:
7778
Max: 804
7879

79-
# Offense count: 7
80+
# Offense count: 8
8081
# Configuration parameters: AllowedMethods, AllowedPatterns.
8182
Metrics/PerceivedComplexity:
8283
Max: 13
@@ -145,7 +146,7 @@ RSpec/EmptyExampleGroup:
145146
- 'spec/cucumber/filters/activate_steps_spec.rb'
146147
- 'spec/cucumber/running_test_case_spec.rb'
147148

148-
# Offense count: 82
149+
# Offense count: 74
149150
# Configuration parameters: CountAsOne.
150151
RSpec/ExampleLength:
151152
Max: 58
@@ -177,12 +178,12 @@ RSpec/ExpectInHook:
177178
- 'spec/cucumber/multiline_argument/data_table_spec.rb'
178179
- 'spec/cucumber/runtime/meta_message_builder_spec.rb'
179180

180-
# Offense count: 6
181+
# Offense count: 13
181182
RSpec/ExpectOutput:
182183
Exclude:
183184
- 'spec/cucumber/formatter/interceptor_spec.rb'
184185

185-
# Offense count: 61
186+
# Offense count: 65
186187
# This cop supports safe autocorrection (--autocorrect).
187188
# Configuration parameters: EnforcedStyle.
188189
# SupportedStyles: implicit, each, example
@@ -196,10 +197,11 @@ RSpec/IndexedLet:
196197
- 'spec/cucumber/filters/retry_spec.rb'
197198
- 'spec/cucumber/glue/registry_and_more_spec.rb'
198199

199-
# Offense count: 26
200+
# Offense count: 36
200201
# Configuration parameters: AssignmentOnly.
201202
RSpec/InstanceVariable:
202203
Exclude:
204+
- 'spec/cucumber/formatter/interceptor_spec.rb'
203205
- 'spec/cucumber/formatter/query/hook_by_test_step_spec.rb'
204206
- 'spec/support/shared_context/http_server.rb'
205207

@@ -316,6 +318,13 @@ RSpec/SortMetadata:
316318
Exclude:
317319
- 'compatibility/cck_spec.rb'
318320

321+
# Offense count: 1
322+
# Configuration parameters: Include, CustomTransform, IgnoreMethods, IgnoreMetadata.
323+
# Include: **/*_spec.rb
324+
RSpec/SpecFilePathFormat:
325+
Exclude:
326+
- 'spec/cucumber/formatter/interceptor_spec.rb'
327+
319328
# Offense count: 7
320329
RSpec/StubbedMock:
321330
Exclude:

CHANGELOG.md

+1
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ that need to rely on procedural loading / reloading of files should use method i
2424
### Fixed
2525
- Fixed an issue where a change to one example in compatibility testing wasn't fully adhered to ([luke-hill](https://github.com/luke-hill))
2626
- Fixed Ruby 3.4+ issue where error backtraces weren't being formatted. ([#1771](https://github.com/cucumber/cucumber-ruby/pull/1771) [orien](https://github.com/orien))
27+
- Fix some problematic specs that were leaking state and showcasing an issue on JRuby ([#1783](https://github.com/cucumber/cucumber-ruby/pull/1783) [luke-hill](https://github.com/luke-hill))
2728

2829
### Removed
2930
- `StepDefinitionLight` associated methods. The class itself is present but deprecated

spec/cucumber/formatter/interceptor_spec.rb

+135-113
Original file line numberDiff line numberDiff line change
@@ -3,142 +3,164 @@
33
require 'spec_helper'
44
require 'cucumber/formatter/interceptor'
55

6-
module Cucumber
7-
module Formatter
8-
describe Interceptor::Pipe do
9-
let(:pipe) { instance_spy(IO) }
10-
11-
describe '#wrap!' do
12-
it 'raises an ArgumentError if its not passed :stderr/:stdout' do
13-
expect { described_class.wrap(:nonsense) }.to raise_error(ArgumentError)
14-
end
15-
16-
context 'when passed :stderr' do
17-
before :each do
18-
@stderr = $stderr
19-
end
20-
21-
after :each do
22-
$stderr = @stderr
23-
end
24-
25-
it 'wraps $stderr' do
26-
wrapped = described_class.wrap(:stderr)
27-
28-
expect($stderr).to be_instance_of described_class
29-
expect($stderr).to be wrapped
30-
end
31-
end
32-
33-
context 'when passed :stdout' do
34-
before :each do
35-
@stdout = $stdout
36-
end
37-
38-
after :each do
39-
$stdout = @stdout
40-
end
41-
42-
it 'wraps $stdout' do
43-
wrapped = described_class.wrap(:stdout)
44-
45-
expect($stdout).to be_instance_of described_class
46-
expect($stdout).to be wrapped
47-
end
48-
end
6+
describe Cucumber::Formatter::Interceptor::Pipe do
7+
let(:pipe) { instance_spy(IO) }
8+
9+
describe '#wrap!' do
10+
it 'raises an ArgumentError if its not passed :stderr/:stdout' do
11+
expect { described_class.wrap(:nonsense) }.to raise_error(ArgumentError)
12+
end
13+
14+
context 'when passed :stderr' do
15+
before :each do
16+
@stderr = $stderr
4917
end
5018

51-
describe '#unwrap!' do
52-
before :each do
53-
@stdout = $stdout
54-
$stdout = pipe
55-
@wrapped = described_class.wrap(:stdout)
56-
end
19+
after :each do
20+
$stderr = @stderr
21+
end
5722

58-
after :each do
59-
$stdout = @stdout
60-
end
23+
it 'wraps $stderr' do
24+
wrapped = described_class.wrap(:stderr)
6125

62-
it "raises an ArgumentError if it wasn't passed :stderr/:stdout" do
63-
expect { described_class.unwrap!(:nonsense) }.to raise_error(ArgumentError)
64-
end
26+
expect($stderr).to be_instance_of described_class
27+
expect($stderr).to be wrapped
28+
end
29+
end
6530

66-
it 'resets $stdout when #unwrap! is called' do
67-
interceptor = described_class.unwrap! :stdout
31+
context 'when passed :stdout' do
32+
before :each do
33+
@stdout = $stdout
34+
end
6835

69-
expect(interceptor).to be_instance_of described_class
70-
expect($stdout).not_to be interceptor
71-
end
36+
after :each do
37+
$stdout = @stdout
38+
end
7239

73-
it 'noops if $stdout or $stderr has been overwritten' do
74-
$stdout = StringIO.new
75-
pipe = described_class.unwrap! :stdout
76-
expect(pipe).to eq $stdout
40+
it 'wraps $stdout' do
41+
wrapped = described_class.wrap(:stdout)
7742

78-
$stderr = StringIO.new
79-
pipe = described_class.unwrap! :stderr
80-
expect(pipe).to eq $stderr
81-
end
43+
expect($stdout).to be_instance_of described_class
44+
expect($stdout).to be wrapped
45+
end
46+
end
47+
end
8248

83-
it 'disables the pipe bypass' do
84-
buffer = '(::)'
85-
described_class.unwrap! :stdout
49+
describe '#unwrap!' do
50+
before :each do
51+
@stdout = $stdout
52+
$stdout = pipe
53+
@wrapped = described_class.wrap(:stdout)
54+
@stderr = $stderr
55+
end
8656

87-
@wrapped.write(buffer)
57+
after :each do
58+
$stdout = @stdout
59+
$stderr = @stderr
60+
end
8861

89-
expect(@wrapped.buffer_string).not_to end_with(buffer)
90-
end
91-
end
62+
it "raises an ArgumentError if it wasn't passed :stderr/:stdout" do
63+
expect { described_class.unwrap!(:nonsense) }.to raise_error(ArgumentError)
64+
end
9265

93-
describe '#write' do
94-
let(:buffer) { 'Some stupid buffer' }
95-
let(:pi) { described_class.new(pipe) }
66+
it 'resets $stdout when #unwrap! is called' do
67+
interceptor = described_class.unwrap! :stdout
9668

97-
it 'writes arguments to the original pipe' do
98-
expect(pipe).to receive(:write).with(buffer) { buffer.size }
99-
expect(pi.write(buffer)).to eq buffer.size
100-
end
69+
expect(interceptor).to be_instance_of described_class
70+
expect($stdout).not_to be interceptor
71+
end
10172

102-
it 'adds the buffer to its stored output' do
103-
expect(pipe).to receive(:write).with(buffer)
73+
it 'noops if $stdout or $stderr has been overwritten' do
74+
$stdout = StringIO.new
75+
pipe = described_class.unwrap! :stdout
76+
expect(pipe).to eq($stdout)
10477

105-
pi.write(buffer)
78+
$stderr = StringIO.new
79+
pipe = described_class.unwrap! :stderr
80+
expect(pipe).to eq($stderr)
81+
end
10682

107-
expect(pi.buffer_string).not_to be_empty
108-
expect(pi.buffer_string).to eq buffer
109-
end
110-
end
83+
it 'disables the pipe bypass' do
84+
buffer = '(::)'
85+
described_class.unwrap! :stdout
86+
@wrapped.write(buffer)
11187

112-
describe '#method_missing' do
113-
let(:pi) { described_class.new(pipe) }
88+
expect(@wrapped.buffer_string).not_to end_with(buffer)
89+
end
90+
end
11491

115-
it 'passes #tty? to the original pipe' do
116-
expect(pipe).to receive(:tty?).and_return(true)
117-
expect(pi.tty?).to be true
118-
end
119-
end
92+
describe '#write' do
93+
let(:buffer) { 'Some stupid buffer' }
94+
let(:pi) { described_class.new(pipe) }
12095

121-
describe '#respond_to' do
122-
let(:pi) { described_class.wrap(:stderr) }
96+
it 'writes arguments to the original pipe' do
97+
expect(pipe).to receive(:write).with(buffer) { buffer.size }
98+
expect(pi.write(buffer)).to eq(buffer.length)
99+
end
123100

124-
it 'responds to all methods $stderr has' do
125-
$stderr.methods.each { |m| expect(pi.respond_to?(m)).to be true }
126-
end
127-
end
101+
it 'adds the buffer to its stored output' do
102+
expect(pipe).to receive(:write).with(buffer)
128103

129-
describe 'when calling `methods` on the stream' do
130-
it 'does not raise errors' do
131-
allow($stderr).to receive(:puts)
104+
pi.write(buffer)
132105

133-
described_class.wrap(:stderr)
134-
expect { $stderr.puts('Oh, hi here !') }.not_to raise_exception(NoMethodError)
135-
end
106+
expect(pi.buffer_string).not_to be_empty
107+
expect(pi.buffer_string).to eq(buffer)
108+
end
109+
end
136110

137-
it 'does not shadow errors when method do not exist on the stream' do
138-
described_class.wrap(:stderr)
139-
expect { $stderr.not_really_puts('Oh, hi here !') }.to raise_exception(NoMethodError)
140-
end
141-
end
111+
describe '#method_missing' do
112+
before :each do
113+
@stdout = $stdout
114+
$stdout = pipe
115+
@wrapped = described_class.wrap(:stdout)
116+
@stderr = $stderr
117+
end
118+
119+
after :each do
120+
$stdout = @stdout
121+
$stderr = @stderr
122+
end
123+
124+
let(:pi) { described_class.new(pipe) }
125+
126+
it 'passes #tty? to the original pipe' do
127+
expect(pipe).to receive(:tty?).and_return(true)
128+
expect(pi.tty?).to be true
129+
end
130+
end
131+
132+
describe '#respond_to' do
133+
before :each do
134+
@stdout = $stdout
135+
$stdout = pipe
136+
@wrapped = described_class.wrap(:stdout)
137+
@stderr = $stderr
138+
end
139+
140+
after :each do
141+
$stdout = @stdout
142+
$stderr = @stderr
143+
end
144+
145+
let(:pi) { described_class.wrap(:stderr) }
146+
147+
it 'responds to all methods $stderr has' do
148+
expect(pi).to respond_to(*$stderr.methods)
149+
end
150+
end
151+
152+
describe 'when calling `methods` on the stream' do
153+
it 'does not raise errors' do
154+
allow($stderr).to receive(:puts)
155+
described_class.wrap(:stderr)
156+
157+
expect { $stderr.puts('Oh, hi here !') }.not_to raise_error
158+
end
159+
160+
it 'does not shadow errors when method do not exist on the stream' do
161+
described_class.wrap(:stderr)
162+
163+
expect { $stderr.not_really_puts('Oh, hi here !') }.to raise_error(NoMethodError)
142164
end
143165
end
144166
end

0 commit comments

Comments
 (0)