Repository navigation
Conversation
Pre-existing broken tests in dependant packages were causing CI to fail
on PRs that hadn't touched those packages. This made it impossible to
get green CI signals on downstream PRs.
Changes:
- Split the failed package list into 'failed_direct' (packages the PR
changed) and 'failed_dependant' (packages that only depend on changed
ones). Only exit(1) for direct failures; dependant failures print a
warning section instead.
- Fix 5 pre-existing broken tests that used mismatched fake paths
(Path('/fake/repo/package1')) causing relative_to() to raise; replaced
with data_path / 'test_integration' to match mock return values.
- Add 2 new tests covering the direct/dependant failure split:
test_dependant_failure_is_warning and
test_direct_failure_exits_nonzero_with_dependant_failures.
Fixes run-llama#21607
codespell (run by pre-commit in CI lint) flags the British spelling "dependant" as a typo and recommends "dependent". This commit renames the identifiers and strings introduced by the previous commit to use the recommended form so the lint check passes. Renames: - failed_dependant -> failed_dependent (variable in llama-dev/llama_dev/test/__init__.py) - _mocked_dependant_fails -> _mocked_dependent_fails (helper in tests) - test_dependant_failure_is_warning -> test_dependent_failure_is_warning - test_direct_failure_exits_nonzero_with_dependant_failures -> test_direct_failure_exits_nonzero_with_dependent_failures - dependant_pkg -> dependent_pkg (local fixture in both new tests) - "dependant packages had pre-existing test failures" string is now "dependent packages had pre-existing test failures" (warning + 2 asserts kept in sync). Pre-existing plural form (get_dependants_packages, dependants_packages, dependants comment) is unchanged - codespell does not flag it and renaming it is out of scope for this PR. All 22 tests in llama-dev/tests/test/test_test.py still pass.
…://github.com/dgenio/llama_index into feature/21607-dependant-test-failures-warning
|
Ngl I would rather not do this, and just fix fhe actual tests 😅 |
|
Totally fair pushback — quick clarification first: the broken tests still run and their failures still print, just demoted from Would you rather I (a) keep this PR as a stopgap plus open a tracking issue for per-package fixes, or (b) close this and pursue per-package fixes directly? Either works — happy to follow your call. |
Addresses audit findings from the PR review cycle: F1 (Major / audit Must-fix): the previous commit changed the `llama-dev test` exit-code contract from "any failure -> exit 1" to "direct failure -> exit 1" with no escape hatch. Add a --strict flag (default False) that restores the old behavior so release-validation or strict-mode pipelines have a way to opt back in. F2 (Should-fix -> audit Must-fix): the dependent-failure warning was emitted only through Rich's console.print, which is easy to miss in collapsed CI logs. Emit a plain GitHub Actions workflow command (`::warning::...`) so the annotation surfaces in the PR Checks tab regardless of TTY handling. F3 (Minor): add two edge-case tests: - test_dependent_success_no_warning: when dependents are present and all pass, no warning section is printed and no ::warning:: emitted. - test_strict_flag_exits_nonzero_on_dependent_failures: with --strict, a dependent-only failure exits 1 and the strict-mode message is printed. Also asserted the ::warning:: annotation in the two pre-existing dependent tests so future regressions in the annotation surface in the test diff. All 24 tests pass; codespell clean.
|
This PR is stale because it has been open 50 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
This PR was closed because it has been stalled for 10 days with no activity. |
Description
Fixes #21607.
Pre-existing broken tests in dependent packages were causing the
Unit Testing / test (3.10|3.11|3.12)matrix to fail on every PR that touchedllama-index-core— becausellama-dev testcomputes the dependent packages of changed packages and tests them too. Any PR that changed core inherited ~23 broken integration test failures it couldn't fix, making it impossible to get a green CI signal.This PR implements Option 2 from the issue: distinguish between failures in directly-changed packages (hard failures) and failures in dependent-only packages (warnings by default, hard failures with
--strict). Only direct failures causeexit(1)in the default mode.New Package?
Did I fill in the
tool.llamahubsection in thepyproject.tomland provide a detailed README.md for my new integration or package?Version Bump?
Did I bump the version in the
pyproject.tomlfile of the package I am updating? (Except for thellama-index-corepackage)llama-devpackage — internal tooling, no version bump needed per repo policy)Type of Change
How Has This Been Tested?
Test commands and results
Changes
llama-dev/llama_dev/test/__init__.pyfailed_direct(packages whose path is inchanged_packages) andfailed_dependent(everything else that failed).exit(1)only fires whenfailed_directis non-empty.⚠️warning section listing the affected packages but do not block CI by default.--strictCLI flag restores the old "any failure → exit 1" contract. With--strict, dependent-only failures also causeexit(1)and aFailing due to --strictmessage prints. Default remains the new lenient behavior.::warning::workflow command so dependent-package regressions surface in the PR Checks tab, not only in the collapsed CI test log.llama-dev/tests/test/test_test.pytest_coverage_failures,test_install_failures,test_skip_failures_no_tests,test_skip_failures_unsupported_python,test_success) — they usedPath("/fake/repo/package1")forchanged_packageswhile the mock returneddata_path / "test_integration", causingPath.relative_to()to raise aValueErrorin the progress display loop.test_dependent_failure_is_warning— when only a dependent package fails,exit_code == 0, the warning message appears, and a::warning::annotation is emitted.test_direct_failure_exits_nonzero_with_dependent_failures— when both a direct and a dependent package fail,exit_code == 1and both sections appear in the output.test_dependent_success_no_warning— when dependents are present and all packages pass, no warning section or::warning::annotation is emitted.test_strict_flag_exits_nonzero_on_dependent_failures— with--strict, a dependent-only failure exits 1 and the strict-mode message is printed.Risks
⚠️warning section in stdout and as a GitHub Actions::warning::annotation in the PR Checks tab. Regressions introduced by a PR in a dependent package will still appear in the warning section — they won't be missed, just won't block the PR by default.--strictto restore it.--fail-fast, explicit package names,--cov,--cov-fail-under,--base-ref,--workersall work unchanged. The new--strictflag is opt-in.Suggested Checklist
uv run -- ruff checkanduvx codespellto satisfy lint (pre-commitlintjob in CI is green)