Skip to content

Conversation

BeyondEvil
Copy link
Contributor

No description provided.

Copy link
Contributor

@davehunt davehunt left a comment

Choose a reason for hiding this comment

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

Could you add a .flake8 configuration file to set the max line length to 88, so we match balck's defaults? Also, it looks like we have a deprecation warning showing up and causing the latest test to fail. Is that related to black's changes?

RemovedInPytest4Warning: MarkInfo objects are deprecated as they contain merged marks which are hard to deal with correctly.

@RonnyPfannschmidt
Copy link
Member

the deprecation warning is unrelated to black, it came in a pytest update

rev: stable
hooks:
- id: black
language_version: python3.7
Copy link
Member

Choose a reason for hiding this comment

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

this should be python3 else the hook breaks for users of many linux distros

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for this! I was looking for a way to be more lenient on the language_version but failed. :( @RonnyPfannschmidt

@davehunt
Copy link
Contributor

the deprecation warning is unrelated to black, it came in a pytest update

We tagged yesterday and this failure did not occur. Has there been a pytest release since then?

@davehunt
Copy link
Contributor

It turns out it has been introduced by this patch @RonnyPfannschmidt as we have an ignore for this deprecation, but the changes mean the line of code that raises it has changed. Ideally we'd avoid the deprecation warning entirely, can you take a look @BeyondEvil?

@BeyondEvil
Copy link
Contributor Author

BeyondEvil commented Aug 24, 2018

Not sure how to deal with this (if at all):

/home/travis/build/pytest-dev/pytest-selenium/.tox/flake8/lib/python3.7/site-packages/pycodestyle.py:113: FutureWarning: Possible nested set at position 1
  EXTRANEOUS_WHITESPACE_REGEX = re.compile(r'[[({] | []}),;:]')

https://travis-ci.org/pytest-dev/pytest-selenium/jobs/420219919

https://gitlab.com/pycqa/flake8/issues/437

Any ideas @davehunt ?

Copy link
Contributor

@davehunt davehunt left a comment

Choose a reason for hiding this comment

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

Looks good, feel free to merge.

[tool:pytest]
filterwarnings =
error::DeprecationWarning
ignore:--firefox-\w+ has been deprecated:DeprecationWarning
ignore:MarkInfo:DeprecationWarning:pytest_selenium.drivers.firefox:88
""")
ignore:MarkInfo:DeprecationWarning:pytest_selenium.drivers.firefox
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm okay to approve this here, but I think we shouldn't ignore these warnings. Could you follow up with a patch that changes the firefox_options fixture to not use the deprecated get_marker?

"contained args <{1.args}>".format(level.__class__.__name__, mark)
)
arguments += mark.args
LOGGER.info("Firefox arguments from markers: {}".format(arguments))
Copy link
Contributor Author

@BeyondEvil BeyondEvil Aug 28, 2018

Choose a reason for hiding this comment

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

Maybe this can be behind an if or be a debug instead? Same goes for the one in get_preferences_from_markers @davehunt

Copy link
Contributor

Choose a reason for hiding this comment

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

As we're appending to a list I don't think it's necessary to include this debug. We included it for capabilities because we update the dictionary, which is more likely to cause confusion. We can probably just use something like:

try:
    arguments = []
    [arguments.extend(m.args) for m in node.iter_markers("firefox_arguments")]
    return arguments
except AttributeError:
    arguments = node.get_marker("firefox_arguments")
    return arguments.args if arguments else []

"contained args <{1.args}>".format(level.__class__.__name__, mark)
)
arguments += mark.args
LOGGER.info("Firefox arguments from markers: {}".format(arguments))
Copy link
Contributor

Choose a reason for hiding this comment

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

As we're appending to a list I don't think it's necessary to include this debug. We included it for capabilities because we update the dictionary, which is more likely to cause confusion. We can probably just use something like:

try:
    arguments = []
    [arguments.extend(m.args) for m in node.iter_markers("firefox_arguments")]
    return arguments
except AttributeError:
    arguments = node.get_marker("firefox_arguments")
    return arguments.args if arguments else []

@BeyondEvil BeyondEvil dismissed RonnyPfannschmidt’s stale review September 6, 2018 07:52

His points has been adressed

@BeyondEvil BeyondEvil merged commit 52c8acd into pytest-dev:master Sep 6, 2018
@BeyondEvil BeyondEvil deleted the pre_commit branch September 6, 2018 07:52
BeyondEvil added a commit to BeyondEvil/pytest-selenium that referenced this pull request Sep 16, 2018
…dev#183)

Release 1.14.0 (pytest-dev#184)

pre commit config

fixes by BLACK and badge in readme

Updates based on comments

Fix deprecation and add doc and test

Fix tests that fail locally

requested changes

run tests in parallel

pre commit config (pytest-dev#185)

* pre commit config

* fixes by BLACK and badge in readme

* Updates based on comments

* Fix deprecation and add doc and test

* Fix tests that fail locally

* requested changes

Move credentials to capabilities

mark failing test as xfail

Run tests in parallel using pytest-xdist (pytest-dev#188)

Also marking Firefox extension test as expected to fail due to current incompatibilities with Selenium and web extensions.
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.

3 participants