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

[11.x] Add expectsSearch() assertion for testing prompts that use search() and multisearch() functions #51669

Merged
merged 7 commits into from
Aug 19, 2024

Conversation

JayBizzle
Copy link
Contributor

@JayBizzle JayBizzle commented May 31, 2024

This is just a failing test, hoping for some help to figure out a fix

I have been trying to test a command that first prompts with a basic select then immediately after prompts with a multisearch.

Im not quite sure what is going on, but it seems the assertions get polluted which each others options, which causes the assertion to fail.

As a side note, to even get to this point, I had to add the following line in ConfiguresPrompts...

$answers = Arr::wrap($answers);

otherwise, array_filter errors here....

? array_values(array_filter($answers, fn ($value) => $value !== 'None'))

@jessarcher I was hoping you may be able to shed some light on all of this as I think I have traced this as far as I can for now and its all getting a bit over my head 😕

Here is the PHPUnit output when running this test, why is the What is you name? assertion seeing the option of Dr as that is an option from the next prompt?

Screenshot 2024-05-31 at 15 42 05

Copy link

Thanks for submitting a PR!

Note that draft PR's are not reviewed. If you would like a review, please mark your pull request as ready for review in the GitHub user interface.

Pull requests that are abandoned in draft may be closed due to inactivity.

@donnysim
Copy link
Contributor

From what I gathered:

->with(Mockery::on(function ($argument) use ($question) {
if (isset($this->test->expectedChoices[$question[0]])) {
$this->test->expectedChoices[$question[0]]['actual'] = $argument->getAutocompleterValues();
}
return $argument->getQuestion() == $question[0];
}))

is definitely the culprit as the $argument question does not always match the $question[0] but is always overridden and should be more in line with:

if ($argument->getQuestion() === $question[0] && isset($this->test->expectedChoices[$question[0]])) {
    $this->test->expectedChoices[$question[0]]['actual'] = $argument->getAutocompleterValues();

    return true;
}

return false;

but then you get mockery exception of

Mockery\Exception\InvalidCountException: Method askQuestion(<Closure===true>) from Mockery_1_Illuminate_Console_OutputStyle should be called
 exactly 1 times but called 2 times.

and I have no idea where that expectation is set up as it seems to come from orchestra.

@JayBizzle
Copy link
Contributor Author

@donnysim thanks for the input, yes, I also saw that message when testing, but I was unable to figure out why

@JayBizzle
Copy link
Contributor Author

Don't want to be a pest, but have you got any thoughts on this @jessarcher 🙏 Thanks

@jessarcher
Copy link
Member

Hey @JayBizzle,

The test assertion story for the search and multisearch prompts is a bit complicated at the moment.

We rely on the Symfony fallback when running tests so that the existing console test assertions can be used. For example, the text prompt falls back to using a Symfony Question, which can be tested using the expectsQuestion method.

There is no equivalent Symfony component for the search and multisearch prompts. Instead, the fallback must first ask for the user's search string using a Symfony Question, pass the result to the search/multisearch callback, and finally ask the user to select option(s) using a Symfony ChoiceQuestion.

This means you'd need to use the expectsQuestion method followed by the expectsChoice method to mock a single search/multisearch prompt.

I have updated your test accordingly and added standalone tests for the search and multisearch prompts.

This could likely be improved by adding a new expectsSearch method that automatically handles the above. Ultimately, I'd like to build test helpers into Prompts so that the Symfony fallback isn't required during testing.

The reason you needed Arr::wrap was because your expectsChoice assertion instructed the mock to return a string ('Dr') when it should always return an array (['Dr']).

@jessarcher
Copy link
Member

I've added an expectsSearch method, which works for the search and multisearch prompts (similar to how expectsChoice works for the select and multiselect prompts). The API might be a little confusing due to the number of required parameters and order, but I've tried to keep it consistent with the other methods.

I'd still like to build test assertions directly into laravel/prompts, but hopefully, this will help in the meantime.

@JayBizzle
Copy link
Contributor Author

Really appreciate the input here @jessarcher

I'll give these changes a test against my code base and report back 👍

Thanks

@driesvints
Copy link
Member

@JayBizzle did you figure that one out?

@JayBizzle
Copy link
Contributor Author

Just on a different project at the moment, but hoping to get a chance to check this potential fix next week.

Will update when I know more

Thanks

@JayBizzle
Copy link
Contributor Author

JayBizzle commented Aug 13, 2024

Hey @jessarcher

Sorry it has taken me so long to check out the changes you made. I have finally just got around to testing this out. Looking good so far. Just one thing im not sure why is happening....

I have the following test..

// snip
->expectsSearch(
    'Search for the tables you would like to import',
    'backups/2020-10-08/11:00/fake_table.sql.gz',
    'fake',
    [
        // 'None',
        'backups/2020-10-08/11:00/fake_table.sql.gz',
    ]
)

When the None option is commented out, i get the following PHPUnit failure...

1) Tests\App\Console\Commands\MysqlImporterTest::test_we_can_successfully_import_specific_backup_files
Question "Search for the tables you would like to import" has different options.
Failed asserting that two arrays are equal.
--- Expected
+++ Actual
@@ @@
 Array (
-    0 => 'backups/2020-10-08/11:00/Y8ejb.sql.gz'
+    0 => 'None'
+    1 => 'backups/2020-10-08/11:00/Y8ejb.sql.gz'
 )

I am really not sure where that None option is coming from.

The code i am testing looks pretty much similar to what your tests look like...

$files = collect(multisearch(
    label: 'Search for the tables you would like to import',
    options: fn (string $value) => strlen($value) > 0
        ? $files->filter(fn ($file) => str_contains($file, $value))->values()->toArray()
        : [],
    hint: 'Use the arrow keys and spacebar to select the tables you would like to import.'
));

@jessarcher
Copy link
Member

Hey @JayBizzle,

Laravel inserts the "None" option because Symfony's choice component requires the user to select an option, while Laravel Prompts allows the user not to select anything when the required argument isn't passed. The "None" option ensures the user can still choose nothing when the Symfony fallback is activated.

However, when running tests, the "None" option should not get inserted:

if ($required === false && ! $this->laravel->runningUnitTests()) {
$options = array_is_list($options)
? ['None', ...$options]
: ['' => 'None'] + $options;
if ($default === null) {
$default = 'None';
}
}

Is it possible that your APP_ENV environment variable is set to something other than testing when running unit tests?

@JayBizzle
Copy link
Contributor Author

JayBizzle commented Aug 16, 2024

@jessarcher Thanks for the reply.

You are correct, further up the test I was doing something like this (as these commands we have should only be run in production)...

$this->app->detectEnvironment(fn () => 'production');

Ive refactored my tests to get around this, so this now all works as expected 👍

Thanks again

@JayBizzle JayBizzle changed the title [11.x] [Failing test] expectsChoice issue when testing a multisearch prompt followed by a select prompt [11.x] Add expectsSearch() assertion for testing prompts that use search() and multisearch() functions Aug 16, 2024
@JayBizzle JayBizzle marked this pull request as ready for review August 16, 2024 09:07
@taylorotwell taylorotwell merged commit 81a0b5b into laravel:11.x Aug 19, 2024
28 checks passed
@JayBizzle JayBizzle deleted the patch-3 branch August 19, 2024 07:13
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

Successfully merging this pull request may close these issues.

5 participants