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

[rb] Add aggregate_failures if there is more than one 'expect()'. #7245

Merged

Conversation

RustyNail
Copy link
Contributor

By putting aggregate_failures, we can check them at once, even if all expect() fail.

The files changed in this PullRequest are below.

  • rb / spec / unit / selenium / webdriver / proxy_spec.rb

Addition of aggregate_failures seems to be applicable to other than the file which I changed this time.
If you can fix other files as well, I will make another PullRequest and respond.

Test passes have been verified with Travis CI results.

@twalpole
Copy link
Contributor

twalpole commented May 29, 2019

This would be cleaner if done with the aggregate_failures metadata form rather than the block form.

@RustyNail
Copy link
Contributor Author

@twalpole
Thank you for the advice.
Moved definition to spec / spec_helper.rb (defined in meta data).

@twalpole
Copy link
Contributor

twalpole commented May 29, 2019

Sorry - I should have been clearer. I didn't mean to add it to every test - there are some tests where it doesn't make sense to use failure aggregation (where later expects can't possibly succeed if a previous failure occurs). The tests you were adding it to did make sense and those could have been done by adding the metadata to the test description rather than using the block form

it 'should allow valid options for a manual proxy', :aggregate_failures do
  ...
end

@p0deje p0deje added the C-rb label May 29, 2019
@RustyNail
Copy link
Contributor Author

@twalpole I'm sorry, my understanding was wrong.
Changed the format to add metadata (: aggregate_failures) to tests that check multiple properties.

@twalpole
Copy link
Contributor

LGTM

@twalpole twalpole merged commit e682a2a into SeleniumHQ:master May 30, 2019
@twalpole
Copy link
Contributor

Thanks!

@RustyNail RustyNail deleted the add_aggregate_failures_proxy_spec_rb branch June 13, 2019 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants