Prompt generally signals that the shell is ready

[jablko]

What do you think of this (hopefully) simplified tmux.prepare? (relying on the prompt to signal that the shell is ready)

[junegunn]

I don't think repeating tmux.until { |lines| lines[-1] == '$' } instead of tmux.prepare is an improvement. We can change the implementation of tmux.prepare method to rely on $ prompt (which is probably a more robust way to check) but removing the layer of abstraction is not something we should do.

And it looks like you did a bunch of additional refactorings here, which is nice, but it's a different concern, so it should be in a separate commit (or in a separate pull request).

[jablko]

👍 Will do: I've replaced tmux.until { |lines| lines[-1] == '$' } with tmux.prepare and split apart changes that stand on their own.

[junegunn]

So you removed the additional CTRL-R for toggling sort and changed the assertions accordingly. Can you add checks for the toggling functionality? i.e. match_count should be 2 and lines[-3] should toggle between echo 3rd and echo 3d on CTRL-R.

[jablko]

✔️ Will do! But for the time being I've undone the changes.

[junegunn]

I'd like to avoid this arbitrary delay. This seems to the case where the old Left, Right hack should work well. What do you think?

tmux.send_keys(query, :Left, :Right)
# $ foo^[[D^[[C (not ready)
# $ foo (ready)
tmux.until { |lines| lines[-1] == "$ #{query}" }

[jablko]

I like it! But I tried it out and the intermittent failure returned: https://travis-ci.org/jablko/fzf/jobs/657850503#L534

[junegunn]

Is the link correct?

[jablko]

Yes, the backtrace shows that it errored at test/test_go.rb:1842:in `block in test_ctrl_r_abort', which is: https://github.com/jablko/fzf/blob/28a39ddbc5a8761f151d8b05e3da4b92f98ad812/test/test_go.rb#L1842

So it sends query (in this case a double quote) followed by :Left and :Right. It finds $ #{query} (in this case $ ") so determines that the shell should be ready! It sends C-r but fish still stumbles and doesn't launch fzf 🙍‍♂️

[junegunn]

That's strange. Let me see.

[junegunn]

You should use the equality condition (lines[-1] == "$ #{query}") instead of start_with? which should return true before fish processes left and right arrow keys.

[junegunn]

If this left-right dance turns out to work well, we probably should put it in tmux.prepare.

[jablko]

🤦‍♂️ Sorry I missed .start_with?. Thanks for your patience! However there are still intermittent failures:

• • https://travis-ci.org/junegunn/fzf/jobs/658740657#L635

  • fzf/test/test_go.rb

    Line 1793 in 191c308

    tmux.until { |lines| lines[-1] == "> #{query}" }

• • https://travis-ci.org/jablko/fzf/jobs/658775012#L527
  • https://github.com/jablko/fzf/blob/5cdaf389648d2415d915bd7408ff2c18ebf64356/test/test_go.rb#L1793
• • https://travis-ci.org/jablko/fzf/jobs/658781294#L524
  • https://github.com/jablko/fzf/blob/51fa9973434ebf990f9e9afd692d4283871e25a5/test/test_go.rb#L1793

I used lines[-1] != "$ #{query}^[[D^[[C" or !lines[-1].start_with?("$ #{query}^") instead of lines[-1] == "$ #{query}" because fish sometimes offers an autocompletion: https://travis-ci.org/jablko/fzf/jobs/658726395#L628

[junegunn]

because fish sometimes offers an autocompletion

Hmm, this is painful. One thing I noticed on my machine is that I append a space after double-quote, the suggestion text disappears. So,

tmux.send_keys(query, :Space, :Left, :Right)

might simplify the check. But anyway, this trick doesn't seem to completely prevent fish from not responding on CTRL-R.

There are other test cases for CTRL-R, should we apply the same hack on those cases as well?

We used to wrap these key bindings with retries method,

• fzf/test/test_go.rb

  Lines 1758 to 1762 in d9c6a03

  	retries do
  	tmux.prepare
  	tmux.send_keys 'C-t'
  	tmux.until { |lines| lines.item_count == 100 }
  	end

• fzf/test/test_go.rb

  Lines 1839 to 1843 in d9c6a03

  	retries do
  	tmux.prepare
  	tmux.send_keys 'C-r'
  	tmux.until { |lines| lines.match_count.positive? }
  	end

• fzf/test/test_go.rb

  Lines 1820 to 1824 in d9c6a03

  	retries do
  	tmux.prepare
  	tmux.send_keys :Escape, :c
  	tmux.until { |lines| lines.item_count == 1 }
  	end

but you removed them all in this pull request. Maybe we should keep them?

[jablko]

I think in those cases there's a prompt we can rely on instead?

[junegunn]

How about changing this to lines.match_count >= 1? We can't be 100% sure if our process is the only match on the system.

[jablko]

If there's more than one match can we be sure we'll the right one?

[junegunn]

No, but it will avoid false negatives. Any reason you decided to remove any_include? which should be more robust in that regard? If we would really like to avoid false positives, we could generate a large random integer instead of 12345 and search for that number.

[jablko]

No, but it will avoid false negatives.

Sorry, I didn't think carefully enough! You're right: I've changed it to lines.match_count >= 1 👍

Any reason you decided to remove any_include? which should be more robust in that regard?

Is it more robust? It checks that the process is among the matches but another match might still be selected?

[junegunn]

It is not perfect, but I'm sure we can agree that it is better than not checking at all.

[jablko]

I see your point, how about lines[5].end_with?(' sleep 12345')?

[jablko]

I'm not convinced that the change proposed here reduces the probability of intermittent failures.

I think I've fixed it: The loop below now runs for 50 minutes without failing (it exhausts the Travis CI job time limit): https://travis-ci.org/github/jablko/fzf/builds/670342136

Would you consider giving it another look?

while ruby test/test_go.rb --verbose; do :; done

[jablko]

I've rebased this change and confirmed that an indefinite loop still exhausts the job time limit without failing: https://travis-ci.org/github/jablko/fzf/builds/670732268