Better runner tests

[deivid-rodriguez]

Descoping #334 a bit by focusing only on runner tests.

[MSP-Greg]

@deivid-rodriguez

Well, I was banging my head against the wall becuase I couldn't get anywhere with this. Then I looked at the test results.

As mentioned, I use trunk (my ruby-loco build). It is the only windows build that has both rb-readline and a compiled readline.so file.

By default, rb-readline is enabled. I switched over to the compiled version. Running

bundle exec rake test test/runner_against_valid_program_test.rb -v

I had one failure; it looked encoding related, which isn't unheard of on windows.

Running with rb-readline, it freezes.

This will require some thought. I think I need a break. Thanks, Greg

EDIT: Just ran the whole set, as opposed to the above file, output below:

  1) Failure:
Byebug::RunnerAgainstValidProgramTest#test_run_with_ruby_script_ruby_is_ignored_and_script_passed_instead [E:/GitHub/byebug/test/runner_against_valid_program_te
st.rb:42]:
Expected /Program: C:\/Temp\/byebug_example.rb/
 to match "C:/ruby26_64/bin/ruby.exe:1: invalid multibyte char (UTF-8)\n" +
"Coverage report generated for test_run_with_a_script_and_params_does_not_consume_script_params, test_run_with_a_script_to_debug, test_run_with_a_single_include
_flag, test_run_with_debug_flag, test_run_with_require_flag, test_run_with_ruby_script_ruby_is_ignored_and_script_passed_instead, test_run_with_stop_flag_stops_
at_the_first_line to E:/GitHub/byebug/coverage. 0.0 / 0.0 LOC (100.0%) covered.\n"
.

  2) Skipped:
Byebug::PostMortemTest#test_execution_is_stopped_at_the_correct_line_after_exception [E:/GitHub/byebug/test/post_mortem_test.rb:51]:
See issue #165

  3) Skipped:
Byebug::ListTest#test_lists_file_changes [E:/GitHub/byebug/test/commands/list_test.rb:194]:
Skipped, no message given

475 runs, 570 assertions, 1 failures, 0 errors, 2 skips

[deivid-rodriguez]

Nice find, although I'm not sure what's best now... 🤔 Maybe this is a bug in rb-readline then?

Regarding the failure, funny enough, I started extracting changes in here to separate PR's and the failure you're getting was due to an unrelated change. It should be fixed by #419, I hope.

[MSP-Greg]

Maybe this is a bug in rb-readline then?

I know there's more. I've been avoiding it.

I hacked the readline.so file into 2.5 and 2.4 releases, and locally got the same results as trunk. So, I don't know whether it's time for windows world to consider moving away from rb-readline or not. I could release a gem, although I need to see about 2.2. & 2.3. They may be a bit messier...

[deivid-rodriguez]

It'd be nice to confirm if this is a real problem in byebug on Windows (that byebug's executable is unusable / hangs), or only with the tests (some bad interaction between Open.capture2e and rb-readline). Maybe it's the latter, it sounds like something similar to #63.

I rebased this and I think it should now pass against the compiled readline?

[MSP-Greg]

I tried running tests a few times, something is amiss...

Start the run - tests are running, then dots stop appearing, and I see 'Coverage report generated for MiniTest, ', following by several test method names. No summary, etc. I think saw this once before today. Have tried with and without -v. Tomorrow I'll try file by file, as there aren't that many...

It's shutdown time... Thanks, Greg

[deivid-rodriguez]

Start the run - tests are running, then dots stop appearing, and I see 'Coverage report generated for MiniTest, ', following by several test method names. No summary, etc. I think saw this once before today. Have tried with and without -v. Tomorrow I'll try file by file, as there aren't that many...

Note that simplecov is being launched several times now (one per process). Those method names are there to prevent reports to overwrite reports from other processes. It's weird that you see that output though since it should be captured by capture2e I think.

Also, have a look at https://github.com/deivid-rodriguez/byebug/blob/master/CONTRIBUTING.md#running-the-test-suite. There are several filters that can be applied to test runs, maybe it's useful to you (although I'm not sure it works on Windows...).

Also, make sure you update your fork, I extracted and merged in to master a lot of stuff out of this branch.

Finally, thanks for the effort, I plan to release byebug 10.0.0 as soon as we get these two PR's in! 🎉

[MSP-Greg]

I plan to release byebug 10.0.0 as soon as we get these two PR's in!

No pressure. Just kidding...

This morning I'll just start checking tests. I just ran the commands folder, rb-readline took 16 seconds, readline.so took 121 seconds. Both completed. I noticed that a long time ago, and also last night without really comparing.

So, I'll keep testing until I find the issue. Hopefully, I can then fix it. Otherwise, I'll look for your help.

Aside - A long time ago, the only reason I tried the compiled readline version is that I knew it probably had been quite a while since anyone tried it with windows builds, and I also wondered if tests would pass with it.

[MSP-Greg]

@deivid-rodriguez

1. I haven't spent hours working with byebug, but all seems to be working with windows.

2. I need to build & install the gem locally to check the bin files. I'll check later today.

3. I haven't yet tried remote debug.

4. As mentioned, using the readline ext, all tests run fine, but it doesn't work well with the UI, so users should stay with rb-readline.

5. I've removed byebug from the testing issue, and I'm just working with open3 and rb-readline (which stops/locks like the tests). Haven't found/fixed the issue, will keep at it (later)...

Thanks, Greg

[deivid-rodriguez]

@MSP-Greg Thanks for your tests! Current CI is passing on Windows so that, in addition to your tests, makes me confident that things work reasonably well there. I'll leave this PR out of 10.0.0 and leave it open until we know more about the readline-rb issue.

Regarding the rb-readline + Open3 issue, so I assume you have a reproduction that doesn't use byebug? In that case, I would report it a https://github.com/ConnorAtherton/rb-readline and probably close #63 and follow up at the rb-readline repo?

[MSP-Greg]

@deivid-rodriguez

Sorry, been busy with some things. I've got my simple case working, but it's bypassing quite a bit of rb-readline functionality. So, although the changes to rb-readline work for that, they don't work for runner_against_valid_program_test.rb, as it still hangs. Hope to get back to it later today/tonite...

Thanks, Greg

[MSP-Greg]

@deivid-rodriguez

I might have rb-readline fixed. Just ran tests on the last commit I pulled:

commit 638718adf2f5b3683ee987b29791646436b06f33 (HEAD -> feature/better_runner_tests, deivid-rodriguez/feature/better_runner_tests)
Author: David Rodr<C3><AD>guez <deivid.rodriguez@riseup.net>
Date:   Tue Jan 9 19:04:00 2018 -0300

    With binmode?

Using that, all passed with trunk except for byebug -v and byebug -h, which looked like match/escaping issues. I haven't paid that much attention to what you've done since then, as I've been working on rb-readline. Most of the issue was that it uses windows api calls to read the keyboard, and then I needed a way to determine if io was pipes or it was running under the UI.

Don't know where to go from here. I could push my changes to my rb-readline fork and you could load them into a branch on your fork, or...

Thanks, Greg

[MSP-Greg]

I'm not sure your fix will work with a windows ui, as no_terminal? is always true for windows?.

I added a require for io/console, then at the top of the method:

      begin
        STDOUT.winsize
      rescue
        return @rl_instream.getc || EOF
      end

STDOUT.winsize seems to always raise an error if rb-readline is connected to pipes. At some point, rb-readline needs to be changed for other encodings, so I used getc.

I did check the above with byebug and a regular console window...

Thanks, Greg

[deivid-rodriguez]

@MSP-Greg So my fix doesn't seem correct indeed since it the PR passes using a childprocess based implementation but hangs using Open3 directly. So we'll need yours :)

I say push your changes into your fork, and I'll link this PR to it after that (or you can do it yourself, see how I'm doing it against my fork right now!).

[deivid-rodriguez]

Well, I did add some checks inside no_terminal? as well and a handfull of other changes, but they are not correct as you point out. Have you tried !rl_instream.tty? for finding out about the pipe? Seems simpler...

[MSP-Greg]

@deivid-rodriguez

Maybe it's time to take a break. Appveyor testing on my fork failed. But, starting fresh with the repo (so it used my git rb-readline), it passes locally as below:

  1) Skipped:
Byebug::ListTest#test_lists_file_changes [E:/GitHub/byebug/test/commands/list_test.rb:194]:
Skipped, no message given

  2) Skipped:
Byebug::PostMortemTest#test_execution_is_stopped_at_the_correct_line_after_exception [E:/GitHub/byebug/test/post_mortem_test.rb:51]:
See issue #165

  3) Skipped:
Byebug::RunnerAgainstValidProgramTest#test_run_with_no_quit_flag [E:/GitHub/byebug/test/runner_against_valid_program_test.rb:73]:
Skipped, no message given

477 runs, 571 assertions, 0 failures, 0 errors, 3 skips
Coverage report generated for MiniTest, test_combinations, test_per_test, test_per_test_class, test_run_stops_at_the_first_line_by_default, test_run_with_a_script_and_params_does_not_consume_script_params, test_run_with_a_script_to_debug, test_run_with_a_single_include_flag, test_run_with_an_invalid_script, test_run_wi
th_an_nonexistent_script, test_run_with_debug_flag, test_run_with_fullpath_ruby_is_ignored_and_script_passed_instead, test_run_with_help_flag, test_run_with_linetracing_flag, test_run_with_no_stop_flag_does_not_stop_at_the_first_line, test_run_with_post_mortem_mode_flag, test_run_with_remote_option_only_with_a_port_number, test_run_with_remote_option_with_host_and_port_specification, test_run_with_require_flag, test_run_with_ruby_is_ignored_and_script_passed_instead, test_run_with_several_include_flags, test_run_with_stop_flag_stops_at_the_first_line, test_run_with_version_flag, test_run_without_a_script_to_debug, test_runs, test_with_seed_option, test_with_verbose_option to E:/GitHub/byebug/coverage. 4480 / 4781 LOC (93.7%) covered.

I'm ran with: ruby 2.6.0dev (2018-01-11 trunk 61763) [x64-mingw32]

So, nothing's changed on Appevor (it still locks), but I'm okay locally....

[MSP-Greg]

@deivid-rodriguez

Stop everything (related to Byebug). Locally, I have symlinks for the few rb-readline files from my ruby install folders to my repo. The files are in lib\ruby\site_ruby. Anyway, I swapped back to the orginal files, and I'm failing. Hence, bundler/rubygems appears to not be switching to the files in the git download. Let me work with it a bit...

[MSP-Greg]

All passing. See Appveyor build here.

[deivid-rodriguez]

Thanks for the effort @MSP-Greg. Could you reduce you rb-readline patch to something minimal? Right now it's a +418, -433 patch full of style changes. It's really hard to understand what's fixing the problem.

[MSP-Greg]

Could you reduce you rb-readline patch to something minimal?

Yes, I can do that.

[MSP-Greg]

Could you reduce you rb-readline patch to something minimal?

See commit 0d84b888 or the diff.

Appveyor results here.

I don't think it's necessary to update the MSYS2 system when using trunk in Appveyor. See change here. Also, might add the cache back in.

[MSP-Greg]

@deivid-rodriguez

I'm not sure whether to patch myself or let you, the issue is with appveyor.yml. Ruby load path for rb files is normally (using trunk example):

/lib/ruby/site_ruby/2.6.0
/lib/ruby/site_ruby
/lib/ruby/vendor_ruby/2.6.0
/lib/ruby/vendor_ruby
/lib/ruby/2.6.0

Originally, I copied the rb-readline files into /lib/ruby/site_ruby, but Ruby 2.3 uses /lib/ruby/site_ruby/2.6.0. I updated appveyor.tml as follows:

install:
  - git clone -q --depth=5  --no-tags --branch=byebug https://github.com/MSP-Greg/rb-readline.git C:\rb-readline

  - set n_dir=C:\ruby%ruby_version%\lib\ruby\site_ruby\%r_vers%
  - attrib.exe -r %n_dir%\*.rb
  - del /q %n_dir%\readline.rb
  - del /q %n_dir%\rbreadline.rb
  - copy C:\rb-readline\lib\readline.rb         %n_dir%\readline.rb
  - copy C:\rb-readline\lib\rbreadline.rb       %n_dir%\rbreadline.rb

environment:
  matrix:
    - ruby_version: _trunk
      # needs update every year
      r_vers: 2.6.0
    - ruby_version: 24-x64
      r_vers: 2.4.0
    - ruby_version: 24
      r_vers: 2.4.0
    - ruby_version: 23-x64
      r_vers: 2.3.0
    - ruby_version: 23
      r_vers: 2.3.0

Make sense?

[deivid-rodriguez]

Yeah, I realized that.

So if you specify rb-readline via Gemfile it doesn't work and the vendored files still get used? Does this mean it is imposible to use other rb-readline rather than the vendored one unless you do what you're doing in appveyor.yml? 😞

[deivid-rodriguez]

By the way, now it's too late since it's already sorted out, but feel free to share your work directly here, otherwise we have a mess of duplicated branches and repositories for no reason. That's the whole point of giving you contributor access! :)

[deivid-rodriguez]

I'm going to merge this as soon as the build passes and move on! We can tweak the rb-readline patch, and how to include it later.

Thanks so much @MSP-Greg for the efforts!

[MSP-Greg]

So if you specify rb-readline via Gemfile it doesn't work and the vendored files still get used? Does this mean it is impossible to use other rb-readline rather than the vendored one unless you do what you're doing in appveyor.yml?

This is the first time I've needed to override files in site_ruby, so I really haven't looked further. I will.

By the way, now it's too late since it's already sorted out, but feel free to share your work directly here,

Sorry. I was busy, and I saw a lot of changes. Given that these were windows issues, I thought I'd try and get everything right in my fork, then push.

Regardless, the goal has been reached...

Thanks, Greg