Skip to content

Conversation

@mguida22
Copy link
Collaborator

@mguida22 mguida22 commented Nov 19, 2025

Description of change

Modifies our github workflow to run tests against all commits to master and next branches (though this PR is against master, not next).

This behavior was changed via #11761.

Pull-Request Checklist

  • Code is up-to-date with the master branch
  • This pull request links relevant issues as Fixes #00000
  • There are new or updated unit tests validating the change
  • Documentation has been updated to reflect this change

Summary by CodeRabbit

  • Chores
    • Enhanced the GitHub Actions workflow configuration to automatically trigger tests on all commits pushed to the master and next branches, complementing the existing pull request triggers. This improvement ensures continuous integration runs more frequently, providing faster feedback and broader test coverage across all critical development and deployment branches.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 19, 2025

Walkthrough

Modifies the GitHub Actions workflow to add push event triggers for master and next branches, in addition to the existing pull_request trigger. No changes to job definitions or workflow logic.

Changes

Cohort / File(s) Summary
CI Configuration
\.github/workflows/tests\.yml
Adds push event triggers for master and next branches to workflow execution configuration

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

  • Straightforward configuration change with no logic modifications
  • Single file affected with minimal additions to event triggers

Possibly related PRs

  • typeorm/typeorm#11763: Modifies the same CI workflow file with additional job and validation step changes

Suggested reviewers

  • gioboa
  • naorpeled

Poem

🐰 A rabbit hops through CI gates,
Where workflows now push and await,
To master and next, with speed and grace,
Tests run faster, keeping pace!

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Title check ✅ Passed The title accurately summarizes the main change: adding push event triggers for master and next branches to the GitHub Actions workflow.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@qodo-merge-for-open-source
Copy link

qodo-merge-for-open-source bot commented Nov 19, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-merge-for-open-source
Copy link

qodo-merge-for-open-source bot commented Nov 19, 2025

PR Code Suggestions ✨

No code suggestions found for the PR.

@pkuczynski pkuczynski changed the title run tests on commits to master and next ci: run tests on commits to master and next Nov 19, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
.github/workflows/tests.yml (1)

3-6: Operational note: CI load impact from running tests on every master/next commit.

Running tests on every push to master and next branches will increase CI load. Consider the frequency of commits and potential queue delays. If this becomes problematic, you may want to add rate limiting (e.g., only on tagged releases) or further refine the branch filter.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 02e7b71 and ddca490.

📒 Files selected for processing (1)
  • .github/workflows/tests.yml (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (20)
  • GitHub Check: tests-linux (20) / sap
  • GitHub Check: tests-linux (20) / mssql
  • GitHub Check: tests-linux (20) / oracle
  • GitHub Check: tests-linux (18) / sap
  • GitHub Check: tests-linux (20) / cockroachdb
  • GitHub Check: tests-linux (18) / postgres (14)
  • GitHub Check: tests-linux (18) / sqljs
  • GitHub Check: tests-linux (18) / postgres (17)
  • GitHub Check: tests-linux (18) / oracle
  • GitHub Check: tests-linux (18) / sqlite
  • GitHub Check: tests-linux (18) / better-sqlite3
  • GitHub Check: tests-linux (18) / mysql_mariadb_latest
  • GitHub Check: tests-linux (18) / mysql_mariadb
  • GitHub Check: tests-linux (18) / mssql
  • GitHub Check: tests-windows / sqljs
  • GitHub Check: tests-linux (18) / mongodb
  • GitHub Check: tests-windows / sqlite
  • GitHub Check: tests-windows / better-sqlite3
  • GitHub Check: formatting
  • GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
.github/workflows/tests.yml (1)

3-6: Verify intent: Should tests always run on master/next commits, or only when files change?

The push trigger syntax is correct, but there's a potential misalignment between the PR objective and the implementation. The PR states the goal is to "run tests against all commits to master and next branches," but the jobs are gated by path-based conditions (e.g., line 77: if: contains(needs.detect-changes.outputs.changes, 'src-or-tests')).

This means tests won't run on commits to master/next if no src or test files changed (e.g., doc-only commits). This could be intentional optimization, but it conflicts with the stated goal of testing "all commits."

Consider:

  • If the intent is to always run tests on master/next: Remove or conditionally disable the paths-filter check for push events.
  • If the intent is to optimize CI by skipping tests when not needed: Update the PR description to clarify this.

@alumni
Copy link
Collaborator

alumni commented Nov 19, 2025

Honestly, I would undo the entire PR #11761 (and possibly some of the other commits around that).

Still, the actions don't run in forks, so contributors won't be able to test their changes in CI unless they submit a PR.

Also, detect-changes is not needed since we plan to handle this via NX 🤷🏻

Basically, the entire CI setup was done on purpose the way it was last month ;) We did not really want to change it.

@pkuczynski
Copy link
Collaborator

Still, the actions don't run in forks, so contributors won't be able to test their changes in CI unless they submit a PR.

Actions do run on forks. Example: https://github.com/typeorm/typeorm/pull/11732/checks

Also, detect-changes is not needed since we plan to handle this via NX 🤷🏻

Once you introduce nx, you can always adopt CI to the new flow. #11761 brings immediate benefits for now (like branch protection rules, faster CI for some PRs).

Basically, the entire CI setup was done on purpose the way it was last month ;)

Was it? The last significant changes I can see in the history are from June 2025...

We did not really want to change it.

Yet someone approved it and used it in the GH settings. So "we" didn't want to change it or you didn't want to change it?

@OSA413 OSA413 merged commit 6ed24f8 into typeorm:master Nov 20, 2025
125 of 129 checks passed
ThbltLmr pushed a commit to ThbltLmr/typeorm that referenced this pull request Dec 2, 2025
Co-authored-by: Oleg "OSA413" Sokolov <OSA413@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants