ext/readline: write test for readline CLI

[LamentXU123]

ext/readline/readline_cli.c has little tests resulting to a 42% test coverage. There ain't any (or very few) behavioral tests to the whole CLI system. The test coverage is only coming from bug regression tests.

This PR offers basic behavior tests for ext/readline/readline_cli.c

[devnexen]

looks good but please wait for someone else review, I m definitely not the most appropriate person for this extension.

[iliaal]

php -a echoes stdin back to stdout, so %Atoken%A matches the echoed input as well as the evaluated output. Three of these expectations pass regardless of what the shell does; details inline.

One of them is masking a real bug. readline_cli_completion_readline leaves Uncaught Error: Undefined constant ReadlineCliCompletionClass::COMPLETION_CLASS_CON in its own output and still passes. readline_cli.c:560 passes text, the whole ClassName::PREFIX, to the constant generator while textlen is already cut to the post-:: length, so constants match against the leading characters of the class name. The function branch passes lc_text and works:

php > class Foo { const FooBar = "MATCHED\n"; const Zed = "zed\n"; }
php > echo Foo::Ze        (tab)
php > echo Foo::ZeFooBar;
MATCHED

Foo::Ze should give Zed. Separate issue, but it is why that expectation has nothing to bite on.

[iliaal]

This expectation cannot fail. Set cli.pager to a command that never runs and pager output still appears, echoed from the input line.

A pager that transforms its input pins it instead:

-d cli.pager='tr a-z A-Z'

with %APAGER OUTPUT%A, which nothing but the pager can produce.

[iliaal]

The tr version holds. Disabling the VCWD_POPEN() in readline_shell_write() turns it red, so the expectation bites now.

Unrelated to your change, checking it turned up a real bug: cli.pager writes everything twice. On this head:

php > echo "pager output
php " ";
pager output
PAGER OUTPUT

sapi_cli_single_write() calls cli_shell_write and drops the return, then writes to stdout anyway. The early return it replaced was size_t shell_wrote; if (shell_wrote > -1), always false on an unsigned compare, added in 6c734a6 and deleted as dead code in 22ecd44. So it has doubled since 5.4. I'll open an issue.

Since this test is the first coverage cli.pager has, I'd rather see the real transcript here than %APAGER OUTPUT%A. Then whoever fixes the duplication has to touch it.

[iliaal]

single, double, block, comment, attribute, inside and outside- all appear in the lines written to stdin, so they match the echo whether or not the shell evaluates anything.

PAREN is the one that holds, because strtoupper() makes the output differ from the source. Same trick on the rest would make the file assert what it says it does.

%A3%A is also thin on its own, since a single digit matches a line number or a version string. echo 6/2 + 100; and 103 is harder to hit by accident.

[iliaal]

The echo collision is gone, but the states still aren't pinned. I replaced the ', ", #, / and ? cases in cli_is_valid_code() with a bare valid_end = 0, which deletes sstring, dstring, comment_line, comment_block, the #[ guard and outside outright, and this test still passes. Only brace_count and brackets_count do any work here. bug77812-readline.phpt goes red on that same mutation.

None of the strings or comments in the file contain ;, {, }, ( or ), and those characters are the only thing the string and comment states hide. valid_end is already 0 wherever one of them opens, so the line keeps accumulating with or without the state.

echo strtoupper('semi ; brace } + newline + end'); would bite. Same for a ; inside the /* */.

[iliaal]

The class constant case does not complete. This run leaves Uncaught Error: Undefined constant ReadlineCliCompletionClass::COMPLETION_CLASS_CON in the output and the test still passes, since class constant is matched against the echoed public const line above it.

Giving the constant a value that differs from its declaration, say "CLASSCONST_OK\n", separates the two. The underlying completion bug is noted in the main thread.

[LamentXU123]

This is a bug in the readline extension. I will deal this in a separate PR to make git log useful, as the main purpose of this PR is to add behavioral tests to the readline CLI.

[iliaal]

Splitting it out is right. Post the issue or PR number here once it exists.

With that case gone, case 2/3 with a class in cli_completion_generator() is the one branch these tests don't reach.

[iliaal]

Same problem as the others. Deleting every backslash case from cli_get_prompt() (\\, \n, \t, \e, \v, \b, \>) leaves this green, so nothing here tests an escape sequence. dyn and the unicode warning are the only expectations that hold, and prompt body matches the echoed input line.

The file is already gated on READLINE_LIB === "readline", so a literal expectation like bug77812-readline.phpt works. That pins \b and \> (->- at top level, -{- inside the block) and \v for free.

[LamentXU123]

Okay now this might address all your reviews :)

[iliaal]

readline_cli_prompt.phpt was red on 2435da9, and ba82792 turns it green by asserting the bug.

The INI parser gets the prompt before cli_get_prompt() does: ; opens a comment and " quotes, so -d cli.prompt=... stores

pre\\-\n-\t-\e-\v-\b-\>-\`-\q-éé-`echodyn

The -`-x tail is gone and the single remaining backtick never pairs, so the eval never runs. echodyn in the new expectation is that mangled literal rather than evaluated output. Deleting the zend_eval_stringl() call from cli_get_prompt() outright leaves the file green on ba82792, so nothing covers the dynamic prompt any more. Disabling the backslash cases does still turn it red, so the escapes are fine.

Not the same case as the pager transcript. There the duplicate line is real output from the feature under test, and disabling the pager still turns that test red.

To get the eval running, quote the whole value so the ; survives, and use single quotes inside the backticks, where " does not:

$prompt = 'pre\\\\-\n-\t-\e-\v-\b-\>-\`-\q-' . "\xC3\xA9\xC3\xA9" . '-`echo \'dyn\';`-`-x ';
$proc = proc_open("$php $ini -d " . escapeshellarg("cli.prompt=\"$prompt\"") . " -a", $descriptorspec, $pipes);

With both, the file passes with the 2435da9 expectation unchanged. %c and %d.%d.%s are fine.

[LamentXU123]

This make sense. I've fixed it in the latest commit.

[iliaal]

LGTM, thanks

[LamentXU123]

Thank you!