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

--fork cli option: fix the case of using pytest -k without setting directory path #2688

Merged
merged 2 commits into from
Oct 21, 2021

Conversation

hwwhww
Copy link
Contributor

@hwwhww hwwhww commented Oct 21, 2021

Issue

  1. --fork is action="append" option without a single default value.
    • request.config.getoption("--fork") returns None if directory path is set.
    • request.config.getoption("--fork") raises ValueError if directory path is unset. (Some AttributeError in pytest internally)
  2. If anyone wants to run with -k AND without setting file or path, it raises error:
pytest -k test_slots_1
request = <SubRequest 'run_phases' for <Function test_slots_1>>

    @fixture(autouse=True)
    def run_phases(request):
>       phases = request.config.getoption("--fork")
E       ValueError: no option named 'fork'

p.s. Thank Ori and Danny for reporting it!

How did I fix it

This PR fixes this special case so that it will run tests against ALL_FORKS even if the directory path is unset.

Also, this PR adds _validate_fork_name to verify if the given fork name is valid.

Note: if anyone uses custom options without target file or directory path, it will raise errors. e.g., pytest -k test_slots_1 --bls-type milagro is invalid.

✦9 ➜ pytest -k test_slots_1 --bls-type milagro
ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --bls-type
  inifile: None
  rootdir: /Users/hwwang/ethereum/consensus-specs

Copy link
Contributor

@djrtwo djrtwo left a comment

Choose a reason for hiding this comment

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

thank you! minor it

tests/core/pyspec/eth2spec/test/conftest.py Outdated Show resolved Hide resolved
Co-authored-by: Danny Ryan <dannyjryan@gmail.com>
@hwwhww hwwhww merged commit a89b55d into dev Oct 21, 2021
@hwwhww hwwhww deleted the fork-cli-fix branch October 21, 2021 15:31
@hwwhww hwwhww changed the title --fork cli option: fix the case of using pytest -k without unset directory path --fork cli option: fix the case of using pytest -k without setting directory path Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants