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

Added support for pytest.ini configuration file #11624

Merged
merged 5 commits into from Mar 8, 2021

Conversation

wilsonliam
Copy link
Contributor

@wilsonliam wilsonliam commented Mar 3, 2021

Closes #11526 : this adds support for a pytest.ini config file and an associated test. Pytest does not offer support for directly specifying a path for a config file, though this can be done indirectly using --rootdir=path, which may bring in other issues; however, this can be implemented if need be.

[ci skip-rust]
[ci skip-build-wheels]

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Contributor

@Eric-Arellano Eric-Arellano left a comment

Choose a reason for hiding this comment

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

Excellent! Thanks Liam. I agree with your decision to not automatically set --rootdir.

--

There's a merge conflict, so you'll need to run something like git pull upstream master. Note that it's better to avoid a rebase now that the pull request is up and has been reviewed, whereas rebasing is often nicer when you're still initially iterating locally.

Resolving merge conflicts can be tricky..let me know if it'd help to pair. Personally, I really like Pycharm to help resolve merge conflicts. I've heard good things about Git Kraken: https://www.gitkraken.com. Or you can manually fix, of course.

--

With the PR description, you can delete that auto-generated part about [ci skip-rust] and [ci skip-build-wheels].

target = create_test_target(rule_runner, [GOOD_WITH_PRINT])
result = run_pytest(rule_runner, target, config="[pytest]\naddopts = -s\n")
assert result.exit_code == 0
assert "All good!" in result.stdout and "Captured" not in result.stdout
Copy link
Contributor

Choose a reason for hiding this comment

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

Good test!

type=file_option,
default=None,
advanced=True,
help="Path to pytest.ini or alternative Pytest config file",
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be helpful to include the restrictions you described in the PR description, something like this maybe:

Pytest will attempt to auto-discover the config file, meaning that it should typically be an ancestor of your tests, such as in the build root. Pants will not automatically set --rootdir for you to force Pytest to pick up your config file, but you can manually set --rootdir in [pytest].args. Refer to https://docs.pytest.org/en/stable/customize.html#initialization-determining-rootdir-and-configfile.

When adding this, you'd want to use \n\n in between the first sentence and the next sentence. Use soft wrapping otherwise. For example:

"Name of the remote instance to use by remote caching and remote execution.\n\n"
"This is used by some remote servers for routing. Consult your remote server for "
"whether this should be set.\n\nYou can also use `--remote-auth-plugin` to provide "
"a plugin to dynamically set this value."

Copy link
Contributor

Choose a reason for hiding this comment

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

Bump on this, as it's definitely a gotcha.

Copy link
Contributor Author

@wilsonliam wilsonliam Mar 8, 2021

Choose a reason for hiding this comment

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

Fixed in latest commit!

Edit - typo with commas in string and hyperlink; fixed now.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
Copy link
Sponsor Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

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

LGTM although I'll give other reviewers the final word.

Nice work!

default=None,
advanced=True,
help=(
"Path to pytest.ini or alternative Pytest config file.\n\n"
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

Great detailed help message!

PathGlobs(
globs=[pytest.config] if pytest.config else [],
glob_match_error_behavior=GlobMatchErrorBehavior.error,
description_of_origin="the option `--pytest-config`",
Copy link
Sponsor Contributor

Choose a reason for hiding this comment

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

People are far more likely to express this in Pants config, so I'd say maybe [pytest].config?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, although I'm working on a PR to fix this across the board + improve the error message for unmatched glos. Currently, all the other subsystems use --pytest-config syntax so it's fine for now, and then I'll fix.

@Eric-Arellano
Copy link
Contributor

Thanks Liam! Made it in time for the release today :)

@Eric-Arellano Eric-Arellano merged commit e7e594c into pantsbuild:master Mar 8, 2021
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.

Add support for pytest.ini
3 participants