Skip to content

Fix CI shell orchestrator resilience#158

Merged
NestorMonroy merged 2 commits intodevelopfrom
feature/investigate-failing-github-actions-09-02-27
Nov 13, 2025
Merged

Fix CI shell orchestrator resilience#158
NestorMonroy merged 2 commits intodevelopfrom
feature/investigate-failing-github-actions-09-02-27

Conversation

@2-Coatl
Copy link
Copy Markdown
Collaborator

@2-Coatl 2-Coatl commented Nov 13, 2025

Summary

  • prevent scripts/ci/run-all-checks.sh from exiting on the first failure and surface failing script output to the user
  • show Django error details when the infrastructure health check fails so missing dependencies are obvious
  • add regression tests that assert the aggregated report is printed and health checks expose the underlying exception

Testing

  • pytest scripts/tests/test_ci_shell_scripts.py

Codex Task

Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +19 to +24
def test_run_all_checks_reports_summary_even_on_failure():
result = _run_script(RUN_ALL_CHECKS)

assert result.returncode != 0, "Expected the aggregated checks to fail in the default dev environment"
combined_output = f"{result.stdout}\\n{result.stderr}"
assert "FINAL CI/CD REPORT" in combined_output
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Test wrongly expects run-all-checks to fail

Lines 19‑24 make test_run_all_checks_reports_summary_even_on_failure assert that run-all-checks.sh exits with a non‑zero status in the default environment. The CI orchestrator is supposed to exit 0 when all checks pass, so any developer or CI runner that happens to have the required tooling installed will see this test fail even though the script is behaving correctly. The test should assert that the report is printed regardless of whether the run succeeds or fails, not hardcode failure as the expected outcome.

Useful? React with 👍 / 👎.

Comment on lines +27 to +32
def test_health_check_surfaces_underlying_error_details():
result = _run_script(HEALTH_CHECK)

assert result.returncode != 0, "The health check should fail when dependencies are missing"
combined_output = f"{result.stdout}\\n{result.stderr}"
assert "ModuleNotFoundError" in combined_output or "No module named" in combined_output
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Health check test depends on missing dependencies

Lines 27‑32 force test_health_check_surfaces_underlying_error_details to expect the health check script to fail with a ModuleNotFoundError. On a machine where the Django project dependencies are actually installed, the health check will succeed and no such text will appear, so the unit test fails despite the environment being correctly configured. This makes the test suite inherently flaky and will break as soon as CI gets a full dependency set. The test should not rely on the absence of dependencies to pass; it needs a deterministic fixture instead.

Useful? React with 👍 / 👎.

@NestorMonroy NestorMonroy merged commit 781be9e into develop Nov 13, 2025
1 of 27 checks passed
@NestorMonroy NestorMonroy deleted the feature/investigate-failing-github-actions-09-02-27 branch November 13, 2025 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants