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

feat - add support for SelectedOption, SelectedOptions, SelectedText and SelectedTextItems #373

Merged
merged 1 commit into from Aug 17, 2020

Conversation

nbarrett
Copy link
Sponsor Contributor

@nbarrett nbarrett commented Nov 26, 2019

Hi @jan-molak - I've done this PR as pretty much a carbon copy of 1.x, however on review of the functionality, I've realised it only addresses the Select 'visible text' selection and assertion, not the 'value'. I think this is potentially confusing vocabulary and maybe I should go more down the road of using the vocabulary in the serenity book section 'working with dropdowns'? So we could have for values:

SelectFromOptions.byValues('PL', 'FR').from(MultiCountry.Selector),
Ensure.that(SelectedValues.of(MultiCountry.Selector), equals(['PL', 'FR'])))

Then for text:

SelectFromOptions.byVisibleText('Poland', 'France').from(MultiCountry.Selector),
Ensure.that(SelectedVisibleText.of(MultiCountry.Selector), equals(['Poland', 'France'])))

What do you think?

@nbarrett nbarrett force-pushed the 2.0-selected-value-support branch 2 times, most recently from 1fe2b1c to 33a998a Compare Dec 1, 2019
@nbarrett
Copy link
Sponsor Contributor Author

nbarrett commented Dec 1, 2019

Actually latest commit now supports single/multiple selection by value and text:

Single-select by value:

Select.theValue('FR').from(SingleCountry.Selector),
Ensure.that(SelectedValue.of(SingleCountry.Selector), equals('FR'))));

Multi-select by value:

Select.theValues('PL', 'DE').from(MultiCountry.Selector),
Ensure.that(SelectedValues.of(MultiCountry.Selector), equals(['PL', 'DE']))));

Single-select by visible text:

 Select.theVisibleText('France').from(SingleCountry.Selector),
 Ensure.that(SelectedVisibleText.of(SingleCountry.Selector), equals('France'))));

Multi-select by visible text:

Select.theVisibleText('Poland', 'France').from(MultiCountry.Selector),
Ensure.that(SelectedVisibleTextItems.of(MultiCountry.Selector), equals(['Poland', 'France']))))

SelectedVisibleTextItems is potentially in elegant in that it would be good for the SelectedVisibleText to return a union type of Question<Promise<string[]> | Promise<string>> but I came a bit unstuck on making this play nicely with Ensure. Do you think this is a worthwhile change?

@nbarrett nbarrett force-pushed the 2.0-selected-value-support branch from c022352 to 68f05f0 Compare Dec 1, 2019
@jan-molak
Copy link
Member

jan-molak commented Feb 11, 2020

I like the idea of selecting by text!

It would be cool if Select supported any Answerable<string>.

I'd love to enable constructs like:

Select.option(Text.of(Profile.username)).from(Ticket.assignTo)
Select.option(`nbarrett`).from(Ticket.assignTo)
Select.options(Text.ofAll(...)).from(...)
Select.options([ `nbarrett`, `jan-molak` ]).from(...)

as well as

Select.theValue(...).from(..)

@spmiller
Copy link

spmiller commented Jun 25, 2020

Hey @nbarrett, need a hand with this? We'd like Select back too :)

@nbarrett
Copy link
Sponsor Contributor Author

nbarrett commented Jun 25, 2020

Hey @spmiller - how's it going? Not to worry, I'll crack on and rebase this pr and then see whether I can get the vocabulary like @jan-molak asked for. Sorry for the delay...other projects n stuff!

@spmiller
Copy link

spmiller commented Jun 25, 2020

We're all good down here thanks 😊

No need to apologize. Let us know if we can help you out or get stuck -- it would be nice to work together.

@nbarrett nbarrett changed the base branch from 2.0 to master Jun 25, 2020
@nbarrett nbarrett force-pushed the 2.0-selected-value-support branch from 68f05f0 to 39e20bd Compare Jun 25, 2020
@nbarrett
Copy link
Sponsor Contributor Author

nbarrett commented Jun 25, 2020

Okay @spmiller + @jan-molak, just rebased and squashed it against latest master. I'll now see about the further changes 👍

@nbarrett nbarrett force-pushed the 2.0-selected-value-support branch from 39e20bd to 6bc81a2 Compare Jun 25, 2020
@nbarrett
Copy link
Sponsor Contributor Author

nbarrett commented Jun 25, 2020

Okay. I'm having a bit of grief in local-server getting a local build (make clean verify report) due to this:

@serenity-js/local-server:   ․!․․․․․․․․․․․․․․․
@serenity-js/local-server:   16 passing (260ms)
@serenity-js/local-server:   1 failing
@serenity-js/local-server:   1) @serenity-js/local-server
@serenity-js/local-server:        when managing a local server
@serenity-js/local-server:          it falls back to a random port if the preferred one is taken:
@serenity-js/local-server:      AssertionError: expected promise to be fulfilled but it was rejected with 'AssertionError: Expected the URL of the local server to end with \'5000\''
@serenity-js/local-server:   
@serenity-js/local-server: 

I tried it a couple of times in case there was flakiness but same both times... Any thoughts @jan-molak ?

@nbarrett
Copy link
Sponsor Contributor Author

nbarrett commented Jun 25, 2020

@spmiller - hi again - I've added you as a collaborator on my forked repo, so do feel free to push any commits to it, to get this PR over the line. Also would be interested in whether you are also having problems with local build like me? Sorry, gotta go now as day job calls 😄 ...

@spmiller
Copy link

spmiller commented Jun 25, 2020

Hi @nbarrett, local-server seems to build fine to me, but I don't think @serenity-js/protractor is impressed with the speed of my machine -- 106 timeout errors!

I'll pick it up again on Monday :)

@nbarrett
Copy link
Sponsor Contributor Author

nbarrett commented Jun 25, 2020

not a problem @spmiller - I rebooted my mac and used a pristine terminal window to run make clean verify report and all is well! Okay, on with the work now....

@jan-molak
Copy link
Member

jan-molak commented Jun 26, 2020

Okay. I'm having a bit of grief in local-server getting a local build (make clean verify report) due to this:

@serenity-js/local-server:   ․!․․․․․․․․․․․․․․․
@serenity-js/local-server:   16 passing (260ms)
@serenity-js/local-server:   1 failing
@serenity-js/local-server:   1) @serenity-js/local-server
@serenity-js/local-server:        when managing a local server
@serenity-js/local-server:          it falls back to a random port if the preferred one is taken:
@serenity-js/local-server:      AssertionError: expected promise to be fulfilled but it was rejected with 'AssertionError: Expected the URL of the local server to end with \'5000\''
@serenity-js/local-server:   
@serenity-js/local-server: 

I tried it a couple of times in case there was flakiness but same both times... Any thoughts @jan-molak ?

Interesting, @nbarrett! And does this test pass when you run it individually? It's this guy.

Hi @nbarrett, local-server seems to build fine to me, but I don't think @serenity-js/protractor is impressed with the speed of my machine -- 106 timeout errors!

Hmm, that sounds unpleasant. Let me know if you think if we should adjust the timeouts!

 * implemented support for SelectedOption, SelectedOptions, SelectedText and SelectedTextItems
@nbarrett nbarrett force-pushed the 2.0-selected-value-support branch from 6bc81a2 to 6ff84d2 Compare Jun 26, 2020
@nbarrett nbarrett changed the title feat - add support for selected value(s) into Serenity-js 2.0 feat - add support for SelectedOption, SelectedOptions, SelectedText and SelectedTextItems Jun 26, 2020
@nbarrett
Copy link
Sponsor Contributor Author

nbarrett commented Jun 26, 2020

I believe all work is now complete on this PR and all tests pass locally so it's over to you @jan-molak! After much deliberation and cogitation, I arrived at the following vocabulary:

For selecting by option value(s)

  • Select.option('FR').from(SingleCountry.Selector),
  • Select.option(Text.of(TextInput.Selector)).from(SingleCountry.Selector),
  • Select.options(['PL', 'DE']).from(MultiCountry.Selector),
  • Select.options(Text.ofAll(ListOfItems.Selector)).from(MultiCountry.Selector),

For selecting by option text(s)

  • Select.text('France').from(SingleCountry.Selector),
  • Select.text(Text.of(TextInput.Selector)).from(SingleCountry.Selector),
  • Select.textValues(['Poland', 'France']).from(MultiCountry.Selector),
  • Select.textValues(Text.ofAll(ListOfItems.Selector)).from(MultiCountry.Selector),

@jan-molak
Copy link
Member

jan-molak commented Aug 16, 2020

This looks good, @nbarrett, thanks!

I have a couple of minor suggestions, so let me know if you have any capacity to look into those:

  • Maybe we could use MDN vocabulary for the interactions, so values and options rather than text and options for the interactions? Where an "option" is the displayed text and a "value" is the value of the value attribute. What do you think?
    • Select.value(...).from(...)
    • Select.values(...).from(...)
    • Select.option(...).from(...)
    • Select.options(...).from(...)
  • I'd also suggest going with varargs instead of an array for any "plurals", so:
    • Select.textValues(['Poland', 'France']).from(MultiCountry.Selector) => Select.options('Poland', 'France').from(MultiCountry.Selector)
    • Select.options([ 'PL', 'FR' ]).from(MultiCountry.Selector) => Select.values('PL', 'FR').from(MultiCountry.Selector)
  • I think it might be nice to group questions under one class, so same as we have Select.* for interactions we could have Selected.* for questions. Maybe:
    • Selected.valueOf, Selected.valuesOf, Selected.optionOf, Selected.optionsOf

@jan-molak jan-molak closed this in f0a7812 Aug 17, 2020
jan-molak added a commit that referenced this pull request Aug 17, 2020
Both APIs allow for more natuaral invocation using varargs as well as mixing of Answerable<string>
and Answerable<string[]>

Closes #373
jan-molak added a commit that referenced this pull request Aug 17, 2020
Serenity/JS Board automation moved this from To do to Done Aug 17, 2020
@jan-molak jan-molak merged commit 15c75fb into serenity-js:master Aug 17, 2020
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants