DM-45314: add workflow for pytest #30
Conversation
Co-authored-by: Tim Jenness <[email protected]>
bbrondel
force-pushed
the
tickets/DM-45314
branch
from
0c86980 to
499cbb9
Compare
.github/workflows/pytest.yaml
Outdated
|
|
||
| jobs: | ||
| run_lint: | ||
| uses: lsst/rubin_workflows/.github/workflows/lint.yaml@main |
There was a problem hiding this comment.
I'll remake the comment from #29 that these lines should not be here because there is already a lint.yaml workflow in this repo -- it's just that that repo needs to be modernized to the current standard of a reusable workflow (see my comment on the other PR).
There was a problem hiding this comment.
Please change the lint.yaml to using the modern DM standard. I pointed at the location on a previous review comment.
If you are using black and isort (the pre-commit config implies that and you have configuration for it in your pyproject.toml) please also add a conformance github action to ensure that. You can find a formatters.yaml in daf_butler that should work -- just copy it over.
For a service we usually do not float dependencies but have a requirements.txt file that is generated from dependencies. Then we explicitly regenerate the pins and use those.
@ktlim is this service going to be turned into a phalanx service?
| run: | | ||
| pytest --cov=./ --cov-report=html --html=pytest_report.html --self-contained-html | ||
|
|
||
| - name: Upload test report |
There was a problem hiding this comment.
Please change to using codecov as discussed. I've added the secret to the repo.
Example code at https://github.com/lsst/daf_butler/blob/main/.github/workflows/build.yaml#L92-L98
| python-version: "3.11" | ||
| cache: "pip" | ||
|
|
||
| - name: Editable mode install |
There was a problem hiding this comment.
You may want to consider separating the install of dependencies from the editable install of the package.
|
|
||
| name: Run PyTest | ||
|
|
||
| on: [push, pull_request] |
There was a problem hiding this comment.
Doesn't this run the action twice on a push to a branch that's already in a PR?
| # Environment variables that must be set: | ||
| # DB_HOST DB_PASS DB_USER DB_NAME or POSTGRES_URL | ||
|
|
||
| # Expose the port. | ||
| EXPOSE 8080 | ||
|
|
||
| ENTRYPOINT [ "gunicorn", "-b", "0.0.0.0:8080", "-w", "2", "pqserver:app" ] | ||
| ENTRYPOINT [ "gunicorn", "-b", "0.0.0.0:8080", "-w", "2", "consdb-pq.pqserver:app" ] |
There was a problem hiding this comment.
I'm kind of surprised that this works with a hyphen in the name. If it doesn't, changing to just consdb should be fine, too.
| @@ -198,16 +198,14 @@ def test_flexible_metadata(client): | |||
| assert result["required_keys"] == ["values"] | |||
|
|
|||
| response = client.post( | |||
| "/consdb/insert/latiss", | |||
| "/consdb/insert/latiss/exposure/obs/2024032100003", | |||
There was a problem hiding this comment.
A little weird to have this be different from the values, but perhaps that's an even better test.
|
@timj consdb-pq is deployed under phalanx already (as is consdb-hinfo at the Summit). https://github.com/lsst-sqre/phalanx/tree/main/applications/consdb Among other things, consdb-pq is not supposed to live for a long time, as it should be replaced by Sasquatch + TAP. |
This change adds a GitHub workflow for pytest and makes modifications that allow the tests to run. It does not correct one failing test. This PR includes changes responsive to comments by @ktlim and @timj. It doesn't fully address DM-45314, but I'd like to go ahead and get this in and then add another ticket.