Move to pytest & pyproject - #589
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f535885a92
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
While setuptools is _not_ deprecated, pyproject seems to be the preference for modern python tooling, and similarly for unittest vs pytest. (In general, I'm hoping to split the test suite into several files, so I can better consistently run tests w/o dealing with Google Scholar ratelimiting). I personally use UV, and have included it inside the .gitignore as such, but there are many other options.
f535885 to
e5bf2fd
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e5bf2fd056
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| [tool.setuptools] | ||
| py-modules = [] |
There was a problem hiding this comment.
Setting py-modules = [] makes the setuptools package configuration explicit and therefore disables automatic discovery, but no packages or packages.find configuration replaces the deleted find_packages() call from setup.py. Wheels produced by the publish workflow will consequently contain metadata but not the scholarly/ package, making the published distribution unusable.
Useful? React with 👍 / 👎.
| run: | | ||
| coverage run -m unittest -v test_module.TestScholarlyWithProxy | ||
| coverage run -m pytest -k TestScholarlyWithProxy |
There was a problem hiding this comment.
After uv sync, the development tools are installed in uv's project environment, but this command invokes coverage directly without activating that environment; the same problem affects coverage xml and both commands in pythonpackage.yml. The installed CLI describes uv sync as updating the project environment and uv run as running a command there, so fresh GitHub runners will generally report coverage: command not found unless these invocations use uv run coverage.
Useful? React with 👍 / 👎.
| class TestScholarly: | ||
|
|
||
| @classmethod | ||
| def setUpClass(cls): |
There was a problem hiding this comment.
Once these classes stop inheriting from unittest.TestCase, pytest does not invoke the camel-case setUpClass/tearDownClass hooks; plain pytest classes require setup_class/teardown_class or fixtures. As a concrete failure, test_download_mandates_csv accesses self.mandates_filename, which is now never initialized, while the proxy class's credential and proxy setup is likewise skipped.
Useful? React with 👍 / 👎.
| continue-on-error: true | ||
| run: | | ||
| coverage run -m unittest -v test_module.TestScholarly | ||
| coverage run -m pytest -k TestScholarly |
There was a problem hiding this comment.
The installed pytest --help states that -k names are substring-matched against test names and parent classes, so -k TestScholarly selects both TestScholarly and TestScholarlyWithProxy. The ordinary three-platform job therefore also runs every proxy-only test without the proxy secrets supplied by proxytests.yml, defeating the suite separation and causing unexpected network-heavy failures or timeouts; select the class by an exact node ID or explicitly exclude the proxy class.
Useful? React with 👍 / 👎.
| - name: Check if pytest failed | ||
| if: "steps.pytest.outcome == 'failure'" |
There was a problem hiding this comment.
The test step is declared with id: pytests, but this condition reads steps.pytest.outcome. When a proxy test assertion fails, continue-on-error allows the workflow onward and the nonexistent context value never equals failure, so the final guard does not restore the failing status and a broken proxy suite can be reported as successful.
Useful? React with 👍 / 👎.
| description = "Simple access to Google Scholar authors and citations" | ||
| readme = "README.md" | ||
| requires-python = ">=3.6" | ||
| dependencies = [ |
There was a problem hiding this comment.
The new runtime dependency list drops typing_extensions, although scholarly/data_types.py imports TypedDict from it whenever Python is below 3.8 and this project still declares support for Python 3.6 and tests on 3.7. Any installation where another dependency does not happen to pull it in transitively will fail immediately on import scholarly; it needs to remain a direct, version-marked runtime dependency.
Useful? React with 👍 / 👎.
| - method: uv | ||
| command: sync |
There was a problem hiding this comment.
Fresh evidence in this revision is the replacement method: uv plus command: sync, but Read the Docs' version-2 python.install schema expects supported installation methods such as pip/setuptools with a project path, or a requirements entry; it does not execute arbitrary commands from a command key here. The configuration therefore fails validation rather than fixing the documentation build; uv must be invoked from a supported build job or the project should be installed through a supported method.
Useful? React with 👍 / 👎.
1 participant
Description
While
setuptoolsis not deprecated,pyprojectseems to be the preference for modern python tooling, and similarly forunittestvspytest.(In general, I'm hoping to split the test suite into several files, so I can better consistently run tests w/o dealing with Google Scholar ratelimiting).
We also downgrade our CI to Python
3.6, to match with the dependency constraints associated with this project.I personally use UV, and have included it inside the
.gitignoreas such, but there are many other options.Checklist
developand notmain.I can not get a substantial amount of these tests to run successfully, but the ones that don't time out do: any PR reproduction here would be great. (It seems that Google Scholar's ratelimiting policy has massively spiked).