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

feat: force enable socket CLI flag #186

Merged
merged 3 commits into from Mar 3, 2023

Conversation

mgaitan
Copy link
Contributor

@mgaitan mgaitan commented Jan 13, 2023

I need to have sockets disabled by default in pyproject.yml but still be able to override it from the command line.

so pytest --force-enable-socket takes precedence even when pyproject.yml has

addopts = "--disable-socket"

@miketheman
Copy link
Owner

Hi @mgaitan !
I don't think this approach is inline with the utility of the plugin.
It seems like you're looking to effectively disable the plugin via command line.

This behavior exists in pytest already - see https://docs.pytest.org/en/stable/how-to/plugins.html#deactivating-unregistering-a-plugin-by-name

Have you tried your scenario with pytest -p no:socket ... ?

@miketheman miketheman added the more-info-needed Further information is requested label Feb 1, 2023
@mgaitan
Copy link
Contributor Author

mgaitan commented Feb 6, 2023

disabling the plugin causes the default arguments I already have in pyproject.toml to be unknown and it cannot be executed.

[core] src (local)# pytest -p no:socket shiphero_app/tests/core/utils/test_sqs.py::test_msg_dedup_id_cleanup

ERROR: usage: pytest [options] [file_or_dir] [file_or_dir] [...]
pytest: error: unrecognized arguments: --disable-socket --allow-unix-socket --allow-hosts=172.18.0.0/24
  inifile: /src/pyproject.toml
  rootdir: /src

@miketheman
Copy link
Owner

@mgaitan Thanks for describing your use case. It sounds like the way I'd handle this would be to remove the socket parameters from the config file, and selectively add the --disable-socket params when needed, and thus have the control you're looking for.

@miketheman miketheman closed this Feb 10, 2023
@mgaitan
Copy link
Contributor Author

mgaitan commented Feb 13, 2023

@miketheman Please reconsider this. This is not a strange pattern in command line applications at all.

Actually, many applications honor an environment variable for an argument, but disregard the environment variable in case the argument is explicitly passed.

To give you a closer and more concrete example related to pytest. In our suite we use pytest-vcr [1] to "cache" HTTP interactions of our tests and avoid relying on the internet. In our pyproject.toml we have the config

[tool.pytest.ini_options]
addopts = """\
    -v --vcr-record=none \
    ... 

which is the recommended way to make CI fails if a test is making a request that is not considered in the cassette.

However, when we need to record a new cassette (or update an existing one) this setting should be "overloaded'' and is simply done by passing --vcr-record=all explicitly in the local execution of pytest, without needing to change the config from pyproject.toml

So basically, pyproject.toml config defines the defaults for the project but any explicit cli arguments takes precedence.

[1] Actually this is the reason why I needed to implementd this change. As we are using both pytest-vcr and pytest-socket, whenever I need to record a new cassette I need to allow that internet access. So I want to override both the socket disabling and vcr-record=none on that single pytest execution from CLI.

@miketheman miketheman reopened this Feb 14, 2023
@miketheman miketheman removed the more-info-needed Further information is requested label Feb 14, 2023
@miketheman
Copy link
Owner

@mgaitan Thanks for the deeper explanation - that use case makes a lot more sense. That context might have been useful at the outset to prevent the back and forth. 😄

In this case, I think the CLI arg should reflect what is actually happening, so possibly --override-disable-socket is clearer, as the enable-socket concept is already used for the pytest fixture/decorators.

@codeclimate
Copy link

codeclimate bot commented Feb 25, 2023

Code Climate has analyzed commit 8b07ef4 and detected 0 issues on this pull request.

View more on Code Climate.

@mgaitan
Copy link
Contributor Author

mgaitan commented Feb 25, 2023

@miketheman I picked --force-enable-socket to make it even more explicit. I hope you are happy with it.

Copy link
Owner

@miketheman miketheman left a comment

Choose a reason for hiding this comment

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

Thanks for the updates!

@miketheman miketheman changed the title force enable socket flag feat: force enable socket CLI flag Mar 3, 2023
@miketheman miketheman merged commit fdb6184 into miketheman:main Mar 3, 2023
@miketheman miketheman added the enhancement New feature or request label Jun 21, 2023
@miketheman miketheman added this to the Next Release milestone Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants