Add and expand bind -x READLINE_* env var tests

[KingMob]

Summary

This adds automatic interactive tests of the READLINE_* env vars for bind -x.

Changes

• Add dependency on pyte, as suggested in the pexpect docs.
• Switch to raw r-strings for escaping to simplify and silence linter warnings
• Switch to single-character command sequences
• Disable pexpect echoing in constructor instead of right afterwards with setecho
• Add LINES and COLUMNS environment variables for interactive tests, and associated --num-lines and --num-columns opts

Areas I'd like feedback on

Term Dimensions

In order to use pyte, it requires size params to emulate a terminal of given dimensions. pexpect defaults to 24 lines X 80 columns, which is fine, but it doesn't expose that anywhere (otherwise I would have used it).

How should we set, and share, the dimension info, which is needed by both pexpect and pyte?

Alternatives

1. Hard-coding the dims everywhere - fragile, though tbf, it's unlikely pexpect would ever change the defaults.
2. Add public height/width constants in harness.py - worked, but global state is ehhh...
3. Querying with tput instead of setting the dimensions - may or may not work under pyte, and might still break the tests if the dims were ever too small.
4. Set LINES and COLUMNS env vars before running each test fn - Mimics real shell vars, recommended here as not all terminal apps use ioctl apparently.

I went with 4. I'm not sold on the use of the LINES and COLUMNS env vars to communicate the dimensions to the interactive tests, but all the alternatives seemed worse.

I added the opts for the lines/columns in case anyone wanted to set them at a higher-level, but if you think that's unnecessary clutter, I can remove them.

[andychu]

Thanks for doing this!

Good questions about the terminal size ...

Actually I noticed that too, before I read your initial comments

I don't have a strong opinion right now, but I do think it's nice to be able to set those in tests

I suggested MAYBE the @register params, but is that useful?

I guess we could also do parameterized tests, like @register(dims=[(80, 24), (10, 5)]) to test very small terminals

---

Although IMO one issue with pexpect is that it is hard to debug test failures! I was doing that the other day, related to job control and Ctrl-Z and fg and such

Nevertheless, the tests are VERY useful, because they did catch a bug I introduced, before commiting!

So thanks for getting these tests to work, it looks very useful

[andychu]

This looks good, let me rebuild the Docker image for the interactive job with pyte now!

And then let's see if this passes, and then we can think about the lines/columns interface a bit more

[KingMob]

Using @register

I think @register could be useful, if we can also set the os.environ env vars for LINES/COLUMNS with it. (AFAICT, this is necessary, not just useful.) Otherwise, all the tests need to know that they have to override those envvars themselves.

Parameterized dims is (probably) overkill. I think testing with tiny dims will cause a lot of tests to fail, but I think that says more about the tests than anything else. Most tests just won't be expecting random newlines, but it's not really an interesting failure, right?

OTOH, it might be useful for testing things like table/column layouts? I think you'd know better.

Setting at command-line

Is this worth keeping? If we set dims for individual tests, what's the value of overriding the global default? If we want to use CLI opts to run only some tests with multiple dims, we need to either waste time rerunning tests we don't care about, or write extra code to avoid that.

[andychu]

Woohoo, I added pyte and the tests pass now!

[andychu]

Hm one thing I don't like is using os.environ as sort of a global, but I see why you did that, because the test signature is just sh right now ... they take a single param

Basically you want to initialize this, to the same values the pexpect is using?

screen = pyte.Screen(num_columns, num_lines)

So yeah maybe we can have @register(needs_dimensions=True) ? And then only in that case, os.environ has LINES and COLUMNS?

(We could also be ambitious and add a test_params=None to the signature, but when you need dimensions, it becomes test_params.lines and test_params.columns. I think that avoids globals, but is maybe a bit of a pain)

---

I think it is at least nice to set the dimensions to --num-lines 30 --num-columns 70 or something, so the code isn't "unused", and we know it actually works

i.e. that pexpect and pyte actually get matching values, and it's not just an accident

[andychu]

I do think the flags can be useful -- actually for completion there was a bug when typing past the last column, and having the dimensions be controllable is nice

[KingMob]

OK, I'll keep the CLI flags, and make sure they're actually used here.

How do you want to proceed with the dims? @register(needs_dimensions=True), @register(test_params=...), something else?

[andychu]

The tests look like

@register()
def foo(sh):
  pass

now

So one idea is:

@register(needs_dimensions=True)
def foo(sh, test_params):
  print(test_params.lines, test_params.columns)  # comes from flag values
  pass

That seems possible? It won't break the other tests, because you pass 2 params only if it needs dimensions.

So we only set os.environ['LINES'] when that's true too.

---

It's possible there's something simpler that will suffice, but this doesn't seem too far away from what we have?

[KingMob]

OK, I can add that when I get a moment.

[KingMob]

@andychu

OK, I've updated it.

Latest changes

• A new test_params map is created, and flows down thru the TestRunner fn calls. By default, it's empty. When empty, we call the 1-arity version of the test case fn, like we currently do. If needs_dimensions was @registered, we set those in the test_params, and in that case, call a 2-arity version.
• Move all dimension env var code into a new context manager, remove it from RunOnce, and use only as needed inside actual test bodies.
• Explicitly set the number of lines and cols for the stateful bind tests in stateful.sh
• Add a missing import re - The linter triggered this, but I'm surprised this bug wasn't detected elsewhere

The amount of extra info being stuck in CASES is right on my personal threshold of switching from a tuple to a dict, but I left it for now.

[andychu]

Looks great, thank you!