Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

docs(test): writing integration tests #430

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ben-alkov
Copy link
Member

Creating as a draft cuz it's a little rough, but I wanted to make sure I captured the essence and the specific commands while they were fresh in my head. Everything's on the table! Let me know if there's anything you'd like to do differently.

also describes the local development process:

  • processing test data with cachi2
  • running pytest

Signed-off-by: Ben Alkov [email protected]

Maintainers will complete the following section

  • Commit messages are descriptive enough
  • Code coverage from testing does not decrease and new code is covered
  • Docs updated (if applicable)
  • Docs links in the code are still valid (if docs were updated)

Note: if the contribution is external (not from an organization member), the CI
pipeline will not run automatically. After verifying that the CI is safe to run:

docs/writing_integration_tests.md Outdated Show resolved Hide resolved
docs/writing_integration_tests.md Outdated Show resolved Hide resolved
docs/writing_integration_tests.md Outdated Show resolved Hide resolved
docs/writing_integration_tests.md Outdated Show resolved Hide resolved
docs/writing_integration_tests.md Outdated Show resolved Hide resolved
@ben-alkov ben-alkov closed this Mar 21, 2024
@ben-alkov ben-alkov deleted the docs-test-integration-testing branch March 21, 2024 20:46
@eskultety
Copy link
Member

Why was this closed? I don't see why we wouldn't benefit from having clearer documentation on how to write integration tests.

@ben-alkov
Copy link
Member Author

🤔 I was pruning stale branches in my fork, and completely forgot about this PR... I'll re-open.

@ben-alkov ben-alkov restored the docs-test-integration-testing branch March 22, 2024 13:14
@ben-alkov ben-alkov reopened this Mar 22, 2024
@ben-alkov ben-alkov marked this pull request as ready for review March 22, 2024 13:15
@ben-alkov ben-alkov force-pushed the docs-test-integration-testing branch 4 times, most recently from c75c53f to e532a76 Compare March 28, 2024 20:51
@ben-alkov ben-alkov force-pushed the docs-test-integration-testing branch from e532a76 to 78e3fa9 Compare April 2, 2024 13:59
docs/writing_integration_tests.md Show resolved Hide resolved

It's a good idea to run the whole cachi2 integration test suite, just to make
sure everything still works properly. The command for this, *from inside the
cachi2 repo* is `tox -e integration`. You can also provide
Copy link
Member

Choose a reason for hiding this comment

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

Do we also want to mention the make test-integration shortcut or would that only add noise?

executing e.g.

```bash
podman run --rm -ti -v "~/temp/cachi2-test:~/temp/cachi2-test:z" -w "~/temp/cachi2-test" localhost/cachi2:latest
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need -it if we're only executing commands with cachi2 rather than trying to run an interactive session (which also would not work without overriding the entry point)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Copypasta from somewhere... I'll change it.

It's a good idea to run the whole cachi2 integration test suite, just to make
sure everything still works properly. The command for this, *from inside the
cachi2 repo* is `tox -e integration`. You can also provide
`CACHI2_IMAGE=localhost/cachi2:latest` if you already have a current cachi2
Copy link
Member

Choose a reason for hiding this comment

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

It's good you mention this, but it feels weird that you actually explain the variable after the walkthrough section (next commit) which comes before this one.


At this point, you should be able to test locally.

## Running the test suite
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need another section to describe some general good practices when it comes to running our integration test suite, the contents can live happily in the previous one IMO.

1. Now run

```bash
CACHI2_IMAGE=localhost/cachi2:latest pytest -rA -vvvv --confcutdir=tests/integration --log-cli-level=DEBUG tests/integration/test_foo.py::test_foo_package[foo_incorrect_checksum]
Copy link
Member

Choose a reason for hiding this comment

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

Running with an already built image IMO only makes sense when your debugging an issue, but from a developer's perspective you just added some code to cachi2 that you may have tested locally outside container environment, so you actually want the image to be rebuilt for tests. By all means we have to document the env variable, I just don't think that what you're proposing in the guideline is going to be the case for most invocations during test development.

Comment on lines +23 to +34
Once you have working test sources, you'll need to commit and push them
somewhere that pytest can clone them from.

We *strongly* recommend making a fork, specific to your test, from one of the
repos found under the [cachito test repos][] GitHub org (note that there
are *many* repos there, covering all of the package managers which cachi2 and
Cachito support) [^1].

Once you have a fork, push to a new branch named after your scenario - now
pytest will have a proper commit hash in a repo to which you have complete
access (once your test is complete and passing, you can simply open a PR against
the repo in the [cachito-testing][] org).
Copy link
Member

Choose a reason for hiding this comment

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

This is summarized later in the walkthrough in a step-by-step fashion. Can we merge this into the walkthrough so that it doesn't feel like repeating the same information with slightly different wording?

@ben-alkov ben-alkov force-pushed the docs-test-integration-testing branch from 78e3fa9 to 76339c0 Compare April 10, 2024 14:11
also describes the local development process:

- processing test data with cachi2
- running pytest

Signed-off-by: Ben Alkov <[email protected]>
To be squashed after review
@ben-alkov ben-alkov force-pushed the docs-test-integration-testing branch from 76339c0 to d89a07f Compare April 11, 2024 19:40
@eskultety
Copy link
Member

@ben-alkov do you plan on re-spinning this?

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