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] - Allow Prompt's multiselect prompt to be tested using expectsChoice #51056

Conversation

lioneaglesolutions
Copy link
Contributor

@lioneaglesolutions lioneaglesolutions commented Apr 14, 2024

Dependent on another PR

This PR depends on #51055 and should not be merged until it is merged.

Purpose

This PR will allow the expectsChoice method to be used when a Laravel Prompt multiselect is being used but when required is false.

Failing Test

I'm not quire sure how to approach the fix here so I first created a failing test;
https://github.com/laravel/framework/pull/51056/files#diff-2cba8279071f67c5fee6df1c8f5112974a28f48ef91fe0e821193ed27c6d8e1cR95-R101

Issue

The issue has to do with the fact that an additional option is being added when required === false;

if ($required === false) {
$options = [new PromptOption(null, 'None'), ...$options];
if ($default === []) {
$default = [null];
}
}

Then, ALL options are being passed to the choice component;

$answers = PromptOption::unwrap($this->components->choice($label, $options, $default, multiple: true));

The expectsChoice assertion though does not cater for the additional choice that has been added. I assume this is because the expectsChoice method is interacting directly with the Symfony command that handles required internally on it's own.

Proposed Solutions

I'm unsure how to best handle this because I'm not sure why $options = [new PromptOption(null, 'None'), ...$options]; is required in the first place.

I was hoping @jessarcher could provide some insights as to how I could go about fixing this failing test.

Adjusting the expectation

If we add None to the expection, this makes the test pass but won't work if the user has actually defined None as an option in their arg list as well;

- ->expectsChoice('Which names do you like?', [], ['John', 'Jane', 'Sally', 'Jack'])
+ ->expectsChoice('Which names do you like?', [], ['John', 'Jane', 'Sally', 'Jack', 'None'])

Extract prompt options

This is what I initially tried in this PR but tests were then failing...

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.

@jessarcher jessarcher self-requested a review April 14, 2024 23:13
@jessarcher jessarcher self-assigned this Apr 14, 2024
@jessarcher
Copy link
Member

jessarcher commented Apr 15, 2024

Hey @lioneaglesolutions, thanks for the PR!

I'm unsure how to best handle this because I'm not sure why $options = [new PromptOption(null, 'None'), ...$options]; is required in the first place.

The 'None' option is required because the multiselect prompt allows the user to select 0 or more options (when required: false); however, the Symfony choice component requires the user to select 1 or more options.

The fallbacks serve two purposes:

  1. Provide a UI for users on Windows without WSL where Prompts is currently unable to work.
  2. Allow the existing test assertions to work.

The 'None' option really only serves the first purpose, allowing Windows users to select 0 options.

The PromptOption class is required to work around some other behaviour that differs between Prompts and Symfony. Prompts will return the option value when a list is passed, otherwise, it returns the key. Symfony will return the value when any numerically-keyed array is passed and will only return the key when the array has at least one non-numeric key.

I think a short-term solution would be to not inject the 'None' option (and maybe not even use the PromptOption classes) when running unit tests, but there would probably still need to be some translation on how the default parameter is handled (Prompts expects an array of keys or values depending on the expected output, Symfony expects a comma-separated string of keys).

I think a longer-term solution would be to build test assertions into the Prompts library itself so that Symfony is only used for the Windows fallbacks.

@jessarcher
Copy link
Member

Hey @lioneaglesolutions,

I've created #51078, which should solve this issue 👍

@jessarcher jessarcher closed this Apr 16, 2024
@lioneaglesolutions
Copy link
Contributor Author

Thanks @jessarcher good to see it fix - loved this $this->line('You like nobody.'); 😂

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.

2 participants