❤️ for @avdi who tackled some big hurdles with #92
Here's a list of things to address:
[x] Swap port 5158 with port 0. This will cause the OS to choose an available port every time, which will prevent two instances from binding to the same port and potentially interfering with each other (primarily via race conditions, at present). If we keep the port flag, it will need to be able to override this.
[x] Do something with the port flag
If we keep it (b/c there is some value to the user), then mention it on the help screen
[x] If we don't keep it (b/c it's an implementation detail we don't want the user to depend on), then remove it
Is there a test for the port flag? I don't remember seeing one, if we keep it, make sure there's one in spec/binary/config_spec.rb
(not critical) Change the event consumer to allow multiple event streams. This would allow the child to use a different event stream from its grandchildren, in the event of forking. (extracted to issue #107)
[x] (Important) stop needs to be called on the child process in the ensure block, otherwise it can leave orphan processes.
[x] Make sure there is a failing test -.- there are several that wanted to catch this, why didn't they? Is it reasonable that no tests failed, given that the -INT test was coupled to the old implementation? Is it reasonable that that test was coupled to the implementation instead of the behaviour?
[x] Delete allow_error from the EvalByWritingFiles (or w/e) class, it's no longer used.
[x] Use File.realdirpath instead of File.expand_path in spec/binary/config_spec.rb (ie make it right where it's calculated instead of where it's used)
[x] spec/evaluate_by_moving_files.rb Figure out what to do with the -INT test that is now turned off. I was thinking the example below, but this may be a good time to consider the orphan process thing, too (based on the test name, that should probably be a different test)
it 'can set a timeout, which interrupts the process group and then waits for the events to finish' do
pre = Time.now
result = invoke 'sleep', timeout_seconds: 0.1
post = Time.now
expect(result.timeout?).to eq true
expect(result.timeout_seconds).to eq 0.1
expect(post - pre).to be > 0.1
end
[x] Make the :unless => RSpec::Support::OS.windows RSpec tag first class. Maybe windows: false (prob put it in spec_helper.rb)
[x] (important) Understand what's going on with this binary/utf-8 thing. It's infecting everything! Is there some underlying issue we can fix instead? See my comments on #92 (IIRC, there are more places than just what I commented on, so check the whole diff)
Move as much of features/env/support.rb into Haiti as possible. Ideally, put Haiti on AppVeyor, as well (it needs some love, but it's unpleasant integrating with Cucumber's code) (extracted to issue #108)
[x] Check each test and cuke that are turned off for Windows to make sure they really should be off (vs being used as a way to defer something that does apply to Windows) For cukes, it's the @not-windows tag, for RSpec, search for windows and unless tags.
windows
Here's a list of things to address:
If we keep it (b/c there is some value to the user), then mention it on the help screenIs there a test for the port flag? I don't remember seeing one, if we keep it, make sure there's one inspec/binary/config_spec.rb
(not critical) Change the event consumer to allow multiple event streams. This would allow the child to use a different event stream from its grandchildren, in the event of forking.(extracted to issue #107)stop
needs to be called on the child process in the ensure block, otherwise it can leave orphan processes.-INT
test was coupled to the old implementation? Is it reasonable that that test was coupled to the implementation instead of the behaviour?allow_error
from theEvalByWritingFiles
(or w/e) class, it's no longer used.File.realdirpath
instead ofFile.expand_path
inspec/binary/config_spec.rb
(ie make it right where it's calculated instead of where it's used)[x]
spec/evaluate_by_moving_files.rb
Figure out what to do with the-INT
test that is now turned off. I was thinking the example below, but this may be a good time to consider the orphan process thing, too (based on the test name, that should probably be a different test):unless => RSpec::Support::OS.windows
RSpec tag first class. Maybewindows: false
(prob put it inspec_helper.rb
)features/regression.feature
useIO::NULL
instead of an if statement on"/dev/null"
Great point by @djberg96 in https://github.com/JoshCheek/seeing_is_believing/pull/92#pullrequestreview-12988839Move as much of(extracted to issue #108)features/env/support.rb
into Haiti as possible. Ideally, put Haiti on AppVeyor, as well (it needs some love, but it's unpleasant integrating with Cucumber's code)@not-windows
tag, for RSpec, search forwindows
andunless
tags.