Consolidate the integration test repos to a single one #789
Conversation
slimreaper35
requested review from
a-ovchinnikov,
brunoapimentel,
eskultety,
taylormadore and
ben-alkov
tests/integration/test_data/bundler_everything_present/container/Containerfile
Show resolved
Hide resolved
slimreaper35
force-pushed
the
oss
branch
4 times, most recently
from
117f9ff to
5abef37
Compare
There was a problem hiding this comment.
Not only did this consolidation clean up the mess we have in integration tests (some bits still remaining for a follow up), but by doing so it sped up the execution by at least 13%! (measured solely on the NPM test suite, so extrapolating it to the whole test suite where there are many more test cases may yield even better results).
I left some comments which should be simple to address and we're good to go.
tests/integration/test_data/npm_lockfile3_aliased_deps/container/Containerfile
Show resolved
Hide resolved
tests/integration/test_data/bundler_everything_present/container/Containerfile
Show resolved
Hide resolved
| cmd: List, | ||
| tmpdir: StrPath, | ||
| cmd: list[str], | ||
| tmp_path: Path, |
There was a problem hiding this comment.
This hunk (multiple occurrences) seems like a non-related cosmetic change.
There was a problem hiding this comment.
This hasn't been addressed. Again, we're close to merging so let's not make a deal out of this, but please make sure you perform cosmetic changes like these (if at all) in a separate commit in the future to avoid polluting a diff with unrelated changes, it just prolongs the review itself.
|
I tried to resolve the majority of the comments. I did not rebase, so the GitHub diff (Compare button) should provide all modified lines only by me. EDIT: Adding a comment to git clean |
| cmd: List, | ||
| tmpdir: StrPath, | ||
| cmd: list[str], | ||
| tmp_path: Path, |
There was a problem hiding this comment.
This hasn't been addressed. Again, we're close to merging so let's not make a deal out of this, but please make sure you perform cosmetic changes like these (if at all) in a separate commit in the future to avoid polluting a diff with unrelated changes, it just prolongs the review itself.
|
@slimreaper35 seems like an update to |
For consistency with other package managers that all use the same function names: - test_<pkg-mngr>_packages - test_e2e_<pkg-mngr> Signed-off-by: Michal Šoltis <[email protected]>
Signed-off-by: Michal Šoltis <[email protected]>
This pytest fixture will be used for running integration tests that will be present inside one repository. Scope of the fixture is "session" -> for each pytest run, there will be only one git clone, because each integration test lives in a separate branch. Signed-off-by: Michal Šoltis <[email protected]>
… name Signed-off-by: Michal Šoltis <[email protected]>
Add a simple test to check disabled network access in each Containerfile as an extra RUN command to ensure the hermetic build works correctly. (better safe than sorry) Signed-off-by: Michal Šoltis <[email protected]>
Modify each test module to use a branch (not a git commit) as a reference inside the new integration test repository that contains all integration (e2e) tests for cachi2. Update and improve a few functions inside utils.py module. Drop unnecessary lines from each Containerfile that were very often duplicated along with the comments. They were tightly coupled with our code printing output of specific files inside of directories coming from pytest. Remove unused "repo" test parameter and rename "ref" test parameter to "branch". Closes containerbuildsystem#676 Signed-off-by: Michal Šoltis <[email protected]>
Just run `nox -s generate-test-data` command. Signed-off-by: Michal Šoltis <[email protected]>
Signed-off-by: Michal Šoltis <[email protected]>
I had to update test data because of the recent changes to yarn pedigree patches - db6e6d9 |
f68209c
Maintainers will complete the following section
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:
/ok-to-test(as is the standard for Pipelines as Code)