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

test: exec the codespell executable consistently #3267

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

perillo
Copy link
Contributor

@perillo perillo commented Dec 27, 2023

Currently, in test_basic.py, the run_codespell and run_codespell_stdin functions exec the codespell script from PATH, not the one from the current directory. This means that the codespell_lib package MUST first be installed, before running pytest.

The current behavior is unsafe, since an user running pytest in the global environment may get an error or, worse, may actually try to test an old version. Additionally this behavior is not documented in README.md.

Update the run_codespell and run_codespell_stdin functions to exec the codespell script via python -m codespell_lib. This change will ensure that python will try to search the __main__ module from the current directory first. Copy the codespell_lib directory to the cwd directory, and configure codespell to ignore it using the -S option, in order to make the environment clean.

Ensure that the cwd parameter in run_codespell and run_codespell_stdin is never None, since the codespell script must not be executed from the current directory. For consistency, also make the args parameter required.

TODO

Probably there should be some comments explaining why it is necessary to copy the codespell_lib directory to the cwd directory

@DimitriPapadopoulos
Copy link
Collaborator

DimitriPapadopoulos commented Dec 27, 2023

To do anything with codespell, you do need to install it as described in Development Setup:

pip install -e ".[dev]"

Also, from Calling pytest through python -m pytest:

You can invoke testing through the Python interpreter from the command line:

python -m pytest [...]

This is almost equivalent to invoking the command line script pytest [...] directly, except that calling via python will also add the current directory to sys.path.

Please don't misunderstand, I do not mean this change is incorrect and shouldn't be applied. Simply, I'd rather everything is as "standard" as possible compared to other packages and the relevant Python documentation (setuptools, pytest).

How do other Python packages compare to codespell?

@perillo
Copy link
Contributor Author

perillo commented Dec 27, 2023

To do anything with codespell, you do need to install it as described in Development Setup:

pip install -e ".[dev]"

I missed it, sorry.
However I still think that relying on the PATH environment variable is unsafe.

In my case, as an example, I have codespell from Arch Linux installed in PATH, so when running tests from the dev version I was actually testing an old version causing test_stdin to fail due to --stdin-single-line not available.

Please don't misunderstand, I do not mean this change is incorrect and shouldn't be applied. Simply, I'd rather everything is as "standard" as possible compared to other packages and the relevant Python documentation (setuptools, pytest).

How do other Python packages compare to codespell?

See https://github.com/pypa/flit/blob/main/tests/test_command.py as an example.

However, pypa packages are not consistent.
Hatch, as an example, use click.testing.CliRunner.

@DimitriPapadopoulos
Copy link
Collaborator

DimitriPapadopoulos commented Dec 27, 2023

Then isn't the PATH incorrect? A typical PATH looks like this:

Ubuntu 22.04
$ cat /etc/skel/.profile 
# ~/.profile: executed by the command interpreter for login shells.
# This file is not read by bash(1), if ~/.bash_profile or ~/.bash_login
# exists.
# see /usr/share/doc/bash/examples/startup-files for examples.
# the files are located in the bash-doc package.

# the default umask is set in /etc/profile; for setting the umask
# for ssh logins, install and configure the libpam-umask package.
#umask 022

# if running bash
if [ -n "$BASH_VERSION" ]; then
    # include .bashrc if it exists
    if [ -f "$HOME/.bashrc" ]; then
	. "$HOME/.bashrc"
    fi
fi

# set PATH so it includes user's private bin if it exists
if [ -d "$HOME/bin" ] ; then
    PATH="$HOME/bin:$PATH"
fi

# set PATH so it includes user's private bin if it exists
if [ -d "$HOME/.local/bin" ] ; then
    PATH="$HOME/.local/bin:$PATH"
fi
$ 
$ echo $PATH
/home/username/.local/bin:/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin:/usr/games:/usr/local/games:/snap/bin:/snap/bin
$ 

@perillo
Copy link
Contributor Author

perillo commented Dec 27, 2023

Then isn't the PATH incorrect? A typical PATH looks like this:
Ubuntu 22.04

No. It is the usual PATH for Linux systems, and I have ~/.local/bin in PATH.
The problem is that you need to first install codespell before running tests.

What I mean with unsafe is that some users may forget to install the package again, after updating the source code. This is not a problem when doing CI with github, but it is a problem for a normal user since there is no support for automation (unless I missed it again).

If you want to keep the current status, maybe you should use tox.

@DimitriPapadopoulos
Copy link
Collaborator

DimitriPapadopoulos commented Dec 28, 2023

See Development Mode (a.k.a. “Editable Installs”) in the Setuptools documentation.

If you install in editable mode, you do not need to reinstall the package. Updates are taken into account instantly.

pip install -e ".[dev]"

After a regular installation, if you forget to reinstall after updating the source code, of course the last changes won't be taken into account. However, that's the whole purpose of a regular installation, and a user error. Why try to work around a user error?

I am more interested in tox, or perhaps nox. Repo-Review suggests:

  • PY007: Supports an easy task runner (nox or tox)
    Projects must have a noxfile.py, tox.ini, or tool.hatch.envs/tool.spin/tool.tox in pyproject.toml to encourage new contributors.

Not sure which of tox or nox one should use nowadays when starting from scratch:

@perillo
Copy link
Contributor Author

perillo commented Dec 28, 2023

@DimitriPapadopoulos, I'm fine with the current state, since it is documented.
However, from my experience, it is very confusing when you actually try to test a different function (I only found the problem after reading the source code).

As for nox vs tox, the former use a simple config file while the latter use a Python program. If nox is enough for codespell, then probably it is better to use it.

@perillo perillo force-pushed the exec-codespell-consistently branch 2 times, most recently from be77fde to 122f744 Compare January 4, 2024 18:52
@perillo
Copy link
Contributor Author

perillo commented Jan 4, 2024

Fixed incorrect types in run_codespell and run_codespell_stdin.

Additionally, I also confirmed that using python -m to run a script works correctly inside a tox environment.

Fixed an unused import.

Currently, in test_basic.py, the run_codespell and run_codespell_stdin
functions exec the codespell script from PATH, not the one from the
current directory.  This means that the codespell_lib package MUST first
be installed, before running pytest.

The current behavior is unsafe, since an user running pytest in the
global environment may get an error or, worse, may actually try to test
an old version.  Additionally this behavior is not documented in
README.md.

Update the run_codespell and run_codespell_stdin functions to exec the
codespell script via `python -m codespell_lib`.  This change will ensure
that python will try to search the __main__ module from the current
directory first.  Copy the codespell_lib directory to the cwd directory,
and configure codespell to ignore it using the `-S` option, in order to
make the environment clean.

Ensure that the cwd parameter in run_codespell and run_codespell_stdin
is never None, since the codespell script must not be executed from the
current directory.  For consistency, also make the args parameter
required.
@perillo perillo force-pushed the exec-codespell-consistently branch from 122f744 to d3e25cc Compare January 4, 2024 19:14
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.

2 participants