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

Support Windows #94

Closed
15 tasks done
JoshCheek opened this issue Dec 14, 2016 · 1 comment
Closed
15 tasks done

Support Windows #94

JoshCheek opened this issue Dec 14, 2016 · 1 comment

Comments

@JoshCheek
Copy link
Owner

JoshCheek commented Dec 14, 2016

Here's a list of things to address:

  • 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.

  • 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
    • 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 Change the event consumer to allow multiple event streams #107)

  • (Important) stop needs to be called on the child process in the ensure block, otherwise it can leave orphan processes.

    • 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?
  • Delete allow_error from the EvalByWritingFiles (or w/e) class, it's no longer used.

  • 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)

  • 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
  • Make the :unless => RSpec::Support::OS.windows RSpec tag first class. Maybe windows: false (prob put it in spec_helper.rb)

  • features/regression.feature use IO::NULL instead of an if statement on "/dev/null" Great point by @djberg96 in Add windows support #92 (review)

  • (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 Add windows support #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 Move overrides into Haiti #108)

  • 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.

JoshCheek added a commit that referenced this issue Dec 25, 2016
…le port

This is part of #94

Remove the --port flag. Because we choose an available port, the user shouldn't need
to specify this. Additionally, we don't want the user to depend on it because this
part of the code has been very volatile with major implementation changes like fifteen
times. By removing it, we prevent users from depending on it, which is preferrable due
to its volatility.
JoshCheek added a commit that referenced this issue Dec 25, 2016
JoshCheek added a commit that referenced this issue Dec 25, 2016
This is part of #94

This provieds a shorter abstraction, and allows other platforms (eg JRuby)
to hook into the same criteria.
JoshCheek added a commit that referenced this issue Dec 26, 2016
…mprove tests

It should hopefully no longer be possible for tests to pass when they leave orphan processes.
Also many more of the tests will now work on JRuby and Windows, the only tests that use fork
now are the tests that test fork. Since we aren't using fork, ichannel isn't helpful anymore,
so remove it from dev deps

Part of #94
Addresses a number of concerns from #92
JoshCheek added a commit that referenced this issue Jan 8, 2017
Explained in https://github.com/JoshCheek/seeing_is_believing/wiki/Encodings

* Does a lot of the remaining difficult work in #94
* Closes #93
* Closes #95
* Causes #92 to be considered "fully merged". IOW, I'm ready to merge the
  windows branch into the master branch. Haven't decided yet if I want
  to release as-is or do a bit more cleanup first (eg that last item in #94)
  Probably release it so we don't get stuck in that stupid situation I
  was in before where v3 wasn't released for like a year. It should just
  be a minor version bump anyway.

Maybe I should start maintaining a changelog. I tried to once, but got
annoyed by something or other and deleted it.
@JoshCheek
Copy link
Owner Author

Closing as all the blocking issues have been done and the ones that we can get by without doing are moved to their own separate issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant