Run tests on PR

[ocsmit]

When opening a PR and then making subsequent commits, it would be nice if the test suite + linters were run directly as checks triggered by the PR. Otherwise we are relying on the branch that a user is requesting to merge to follow the naming convention defined in the build-pipeline.yml. E.g. for #433 there was a typo in a type hint that I introduced (xr.DataSet, instead of xr.Dataset) after I ran the tests locally which wasn't caught by myself, during the code review, or by CI/CD on my own branch because it did not follow the naming convention. So this wasn't caught until the code was merged into develop, at which point the tests ran and then obviously failed.

Obviously if I had ran the tests locally after that one word change, I would have caught but the point of having CI/CD is to catch those type of things.

It could be as simple as something like the following

name: lint and test
on: [push, pull_request]

jobs:
  lint:
    runs-on: ubuntu-latest
    steps:
      - name: Checkout code
        uses: actions/checkout@v4

      - name: Set up Python
        uses: actions/setup-python@v5
        with:
          python-version: "3.12"

      - name: Install Poetry
        uses: abatilo/actions-poetry@v4
        with:
          poetry-version: 2.1.3

       - name: Install l2ss-py
        run: poetry install -E harmony
      - name: Lint
        run: |
          poetry run poetry run ruff check podaac/
      - name: Test and coverage
        run: |
          poetry run pytest -v \
            --junitxml=build/reports/pytest.xml \
            --cov=podaac \
            --cov-report=xml:build/reports/coverage.xml \
            --cov-report=term-missing:skip-covered \
            -m "not aws and not integration" \
            tests/

This example has some overlap with the build pipeline so would likely also need revisit build-pipeline.yml. I am open to alternative approaches, but the current state where PRs can be merged without any automated checks having passed is less than ideal.

[tloubrieu-jpl]

Thanks for your feedback @ocsmit , I'll let @sliu008 make a quick analysis on your request and then we can schedule the work to be done.

[owenlittlejohns]

I found my way here through a bit of a tangent, but definitely agree that automated CI/CD checks for executing unit tests are something that should be implemented.

The CI/CD for testing the Harmony services that DAS maintains is pretty cut-and-paste, implemented via this workflow and this other one. They use scripted bash files, with a whole bunch of details available on this wiki page. PO.DAAC services tend to be a little different, so the exact contents of the bash scripts might not work here, but I thought I'd share generally.

Linting would also be a nice thing to add too, as @ocsmit suggests, and I'd recommend using pre-commit.ci. That also means you can have developers install pre-commit locally for a project and minimise things getting pushed up to remote. That's proven to be pretty successful for other teams (I'm a bit of a pre-commit evangelist these days).

I'd be happy to contribute to the CI/CD work for this - putting my TRT lead hat on: automated CI/CD for, at minimum, executing unit tests is something we should be doing for all application code across the train.