Skip to content

Added action to run tests - #163

Open
indykoning wants to merge 5 commits into
masterfrom
feature/tests-action
Open

indykoning wants to merge 5 commits into
masterfrom
feature/tests-action

Conversation

@indykoning

Copy link
Copy Markdown
Owner

Summary

This adds an action to automatically run the tests upon PR, outputting in JUnit format so github can show the error lines in the pr files view

Checklist

  • I've made sure the PR does small incremental changes. (new code additions are dificult to review when e.g. the entire repository got improved codestyle in the same PR.)
  • I've added/updated the relevant docs for code changes i've made.

@johanzander

Copy link
Copy Markdown
Collaborator

Thanks for adding a pytest workflow. A few suggestions on .github/workflows/pytest.yml:

  1. Install the package instead of symlinking (line 16). ln -s ../growattServer tests/growattServer makes CI behave differently from local runs, which fail with ModuleNotFoundError unless the package is installed. The link isn't git-ignored, so it could be committed by accident, and a second ln -s would then fail or nest. pip install -e . fixes this, as does pythonpath = . in the pytest config.
  2. --doctest-modules (line 17). It only runs over tests/, so the library's doctests are never collected. It also picks up the symlinked copy of the package, which can cause "import file mismatch" errors. I'd drop the flag or point it at the installed package.
  3. JUnit output (line 17). junit/test-results.xml is written but never uploaded or used to annotate the PR. If you want error lines in the files view, the workflow needs a reporter step and checks: write permission.
  4. Dependencies (line 14). The workflow installs unpinned pytest and requirements.txt, which can drift from install_requires in setup.py. Installing the package and a test extra gives a single source of truth.
  5. Triggers (line 1). With both push and pull_request and no branch filter, each PR commit is tested twice. The mypy and ruff workflows do the same, so this is optional. An explicit least-privilege permissions: block would also be good.
  6. Nit (line 16). The ln -s line has trailing whitespace.

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