Skip to content

CI/CD for Testing#13

Open
2Ryan09 wants to merge 39 commits intomainfrom
basic-test-infra
Open

CI/CD for Testing#13
2Ryan09 wants to merge 39 commits intomainfrom
basic-test-infra

Conversation

@2Ryan09
Copy link
Copy Markdown

@2Ryan09 2Ryan09 commented Apr 16, 2026

A few design decisions that are up for debate:

  • tests run on pushes to main and PRs opened against main
  • tests are organized by unit and integration (requires live backend)
  • new top-level tests/ directory vs. previous client/tests/
  • migration from legacy requirements.txt to modern pyproject.toml
  • Updated scripts/dev/00-test-users.mongodb to fully seed a test mongodb database
  • formalized use of uv

Diff looks scary, but 1,203 additions were a lock file

@2Ryan09 2Ryan09 requested review from ac6y, swelborn and yee379 April 16, 2026 22:37
Copy link
Copy Markdown
Collaborator

@swelborn swelborn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good, a few comments

Comment thread .github/workflows/ci-cd.yaml
Comment thread .github/workflows/ci-cd.yaml Outdated
Comment thread .github/workflows/ci-cd.yaml
Comment thread .github/workflows/ci-cd.yaml Outdated
Comment thread scripts/README.md
Comment thread main.py
EMAIL_SERVER_HOST = os.getenv( 'COACT_EMAIL_SERVER_HOST', 'smtp.slac.stanford.edu' )
EMAIL_SERVER_PORT = os.getenv( 'COACT_EMAIL_SERVER_PORT', 25 )
ADMINS = re.sub( "\s", "", environ.get("ADMIN_USERNAMES",'')).split(',')
ADMINS = re.sub(r"\s", "", environ.get("ADMIN_USERNAMES",'')).split(',')
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

prob could be split out of this PR but not really big deal

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this (plus the other raw string addition) was simply to clean up the warnings in the test output, I don't believe it was causing any errors

Comment thread models.py
nodecpucount: Optional[int] = UNSET
nodecpucountdivisor: Optional[int] = UNSET
nodegpucount: Optional[int] = UNSET
nodegpucount: Optional[int] = None
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what are ramifications of this

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high level: UNSET tells strawberry a value was unset vs explicitly set to None (similar to JavaScript's undefined.

The generated code in client/ already forces all UNSETs to Nones anyways, so this won't affect that path and why I assumed this was safe, tho there's a point to be made about the raw GraphQL query angle.

These specific values came up because the following was being raised in test_clusters on the newly seeded databases:

_______________________________________ test_clusters _______________________________________

client = <coact.client.client.CoactClient object at 0x10bbef8f0>

    async def test_clusters(client: CoactClient):
>       result = await client.clusters()
                 ^^^^^^^^^^^^^^^^^^^^^^^

tests/integration/test_queries.py:46:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
.venv/lib/python3.12/site-packages/coact/client/client.py:231: in clusters
    data = self.get_data(response)
           ^^^^^^^^^^^^^^^^^^^^^^^
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

self = <coact.client.client.CoactClient object at 0x10bbef8f0>
response = <Response [200 OK]>

    def get_data(self, response: httpx.Response) -> dict[str, Any]:
        if not response.is_success:
            raise GraphQLClientHttpError(
                status_code=response.status_code, response=response
            )

        try:
            response_json = response.json()
        except ValueError as exc:
            raise GraphQLClientInvalidResponseError(response=response) from exc

        if (not isinstance(response_json, dict)) or (
            "data" not in response_json and "errors" not in response_json
        ):
            raise GraphQLClientInvalidResponseError(response=response)

        data = response_json.get("data")
        errors = response_json.get("errors")

        if errors:
>           raise GraphQLClientGraphQLMultiError.from_errors_dicts(
                errors_dicts=errors, data=data
            )
E           coact.client.exceptions.GraphQLClientGraphQLMultiError: Int cannot represent non-integer value: <UnsetType instance>; Int cannot represent non-integer value: <UnsetType instance>; Int cannot represent non-integer value: <UnsetType instance>; Int cannot represent non-integer value: <UnsetType instance>; Expected Iterable, but did not find one for field 'Cluster.memberprefixes'.; Expected Iterable, but did not find one for field 'Cluster.memberprefixes'.

.venv/lib/python3.12/site-packages/coact/client/async_base_client.py:146: GraphQLClientGraphQLMultiError

Comment thread pyproject.toml
Comment thread pyproject.toml Outdated
Comment thread Dockerfile
COPY requirements.txt /app
COPY pyproject.toml /app

RUN pip3 install -r /app/requirements.txt
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could also use uv for this dockerfile but that could be another PR

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done, doing this also made me discover we were using Python 3.10 in prod but it sounds like most people were using 3.12 in dev 🙂

@2Ryan09 2Ryan09 changed the base branch from node-overage-restore to main April 17, 2026 22:27
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.

3 participants