Skip to content

Simplify --splinter-headless #123

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Sep 12, 2020
Merged

Simplify --splinter-headless #123

merged 3 commits into from
Sep 12, 2020

Conversation

tony
Copy link
Member

@tony tony commented May 13, 2019

It feels a bit strange doing true/false for a headless option.
It could be my own aesthetic / tastes.

argparse has 'store_true' for cases like this (see docs):

'store_true' and 'store_false' - These are special cases of 'store_const' used for storing the values True and False respectively. In addition, they create default values of False and True respectively..

So --headless sets it to True.

I can't think of a case where a user would want to accept False considering the default behavior is that, but if they did, they can always add their own option via pytest_addoption)

Not entering --splinter-headless infers False (this is backed by default values in pytest-splinter and probably user intuition). Entering --headless implies the user wants to run in headless mode.

@coveralls
Copy link

coveralls commented May 13, 2019 •
edited
Loading

Coverage Status

Coverage remained the same at 87.681% when pulling ef8208d on tony:headless-argparse into a3406f1 on pytest-dev:master.

@bubenkoff bubenkoff requested a review from youtux May 14, 2019 12:30
@youtux
Copy link
Contributor

youtux commented May 14, 2019

Sorry @bubenkoff, I don't have time to check this and the possible incompatibilities that may arise.

@tony
Copy link
Member Author

tony commented May 14, 2019 •
edited
Loading

@youtux @bubenkoff

I doubt there's a side effect.

Here's two examples of --headless being used as in this PR: https://github.com/seleniumbase/SeleniumBase/blob/v1.23.9/seleniumbase/plugins/selenium_plugin.py#L87, pytest-dev/pytest-selenium#135 (comment)

via SeleniumBase's README:

If you want to run tests headlessly, use --headless, which you'll need to do if your system lacks a GUI interface. Even if your system does have a GUI interface, it may still support headless browser automation.

As for this project in particular:

pytest-splinter defaults to False for headless (as a user would probably expect)

It's really unintuitive to pass true/false to an option when the default behavior is false.

There are corner cases (as always), but nothing is stopping them from writing their own option (e.g. --no-headless) via pytest_addoption or customizing the splinter_headless fixture in their conftest. We could also introduce --no-headless at a later time, but it's doubtful a request would arise.

@tony
Copy link
Member Author

tony commented May 31, 2019

@youtux @bubenkoff

This is behavior splinter should use by default. I think we should merge this.

There's no point in --splinter-headless false because splinter_headless option already defaults to false.

@tony tony force-pushed the headless-argparse branch from e61c6eb to 78afa2a Compare July 20, 2019 22:37
@tony
Copy link
Member Author

tony commented Jul 20, 2019

Hi @youtux @bubenkoff, checking in, just rebased.

The reason I stand behind this is it's simpler.

Omission of the flag generally implies falsiness. Going by the https://en.wikipedia.org/wiki/Principle_of_least_astonishment.

There's no point in --splinter-headless accepting false if splinter_headless defaults to False.

At this point I already written my own --headless to get the equivalent behavior. It just is a bit odd to have a flag set to false when it defaults to false 😆

If it's not okay to put in it's okay to close this.

bubenkoff
bubenkoff requested changes Sep 11, 2020
View reviewed changes
Copy link
Member

@bubenkoff bubenkoff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tony please resolve conflicts

@tony
Copy link
Member Author

tony commented Sep 11, 2020

@bubenkoff Doing so now.

@tony tony force-pushed the headless-argparse branch 2 times, most recently from 6eedc24 to c07d227 Compare September 11, 2020 21:56
@tony
Copy link
Member Author

tony commented Sep 11, 2020

@bubenkoff Done

bubenkoff
bubenkoff requested changes Sep 11, 2020
View reviewed changes
Copy link
Member

@bubenkoff bubenkoff left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please:

  • fix the documentation
  • add changelog record
  • add authors record
    also tests are broken with this

@tony tony force-pushed the headless-argparse branch 2 times, most recently from 5e84e97 to 6d92c5b Compare September 12, 2020 17:56
tony added 3 commits September 12, 2020 13:10
@tony
Simplify --splinter-headless
a7ce0d1 
Not entering --splinter-headless infers False (this is backed by
default values in pytest-splinter and probably user intuition).
Entering --headless implies the user wants to run in headless mode.
@tony
Document --splinter-headless behavior
2236577 
Without argument, behavior is unchanged. `--splinter-headless` without
arguments defaults to 'true'. Optionally false or true can be passed.
@tony
Update authors and changelog
ef8208d
@tony tony force-pushed the headless-argparse branch from 6d92c5b to ef8208d Compare September 12, 2020 18:10
@tony
Copy link
Member Author

tony commented Sep 12, 2020

@bubenkoff Better now?

@bubenkoff
Copy link
Member

please squash & merge

@tony tony merged commit 51cd9bc into pytest-dev:master Sep 12, 2020
@tony tony deleted the headless-argparse branch September 12, 2020 21:23
@mpasternak mpasternak mentioned this pull request Sep 15, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bubenkoff bubenkoff bubenkoff approved these changes

@youtux youtux Awaiting requested review from youtux

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tony @coveralls @youtux @bubenkoff