Skip to content

Streamlined the unit tests and addressed unit tests bug - #1191

Closed
kennethshsu wants to merge 2 commits into
mainfrom
#1182_unittests_fail_branchname
Closed

Streamlined the unit tests and addressed unit tests bug#1191
kennethshsu wants to merge 2 commits into
mainfrom
#1182_unittests_fail_branchname

Conversation

@kennethshsu

@kennethshsu kennethshsu commented Aug 3, 2026

Copy link
Copy Markdown
Member

Summary of Changes

Fixes 1182, previously, we used "*" only to catch the branch name, which can be escaped by "/", using "**" will fix that.
This fix goes one step further, as we are all familiar, there are WAAY too many tests that run on each PR. This PR will update that to run on every PR and push to main only, it will not rerun main multiple times.

Related GitHub Issue(s)

Closes #1182

Additional Context for Reviewers

Checklist

  • I passed tests locally for both code (uv run pytest) and documentation changes (uv run --directory docs jb build . --builder=custom --custom-builder=doctest)

Note

Low Risk
Only GitHub Actions trigger configuration changed; no application or test logic was modified.

Overview
CI trigger change in pytest.yml: push no longer uses the '*' branch filter (which could miss branches with / in the name). It now runs unit tests only on pushes to main, while pull_request still runs the full matrix on every PR.

This cuts redundant workflow runs on feature branches and avoids duplicate runs when main is updated, without changing what runs on PRs.

Reviewed by Cursor Bugbot for commit cc2051f. Bugbot is set up for automated code reviews on this repo. Configure here.

@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

Pyright Type Completeness

View the full pyright --verifytypes output for this commit

Project (full chainladder package, at this PR's head): 15.0% of exported symbols fully typed (196 / 1305)

Known Ambiguous Unknown Total
Project (head) 196 110 999 1305

Other symbols referenced but not exported by chainladder: 13

Known Ambiguous Unknown Total
Other (head) 3 1 9 13

Symbols without documentation:

  • Functions without docstring: 315
  • Functions without default param: 0
  • Classes without docstring: 10

Patch (exported symbols added or changed by this PR): no exported symbol type-completeness changes detected.

@codecov

codecov Bot commented Aug 3, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.14%. Comparing base (8461cfb) to head (cc2051f).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1191   +/-   ##
=======================================
  Coverage   91.14%   91.14%           
=======================================
  Files          91       91           
  Lines        5365     5365           
  Branches      681      681           
=======================================
  Hits         4890     4890           
  Misses        340      340           
  Partials      135      135           
Flag Coverage Δ
unittests 91.14% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@kennethshsu kennethshsu changed the title Streamed line the unit tests and addressed unit tests bug Streamlined the unit tests and addressed unit tests bug Aug 3, 2026
@henrydingliu

Copy link
Copy Markdown
Member

i'm a little worried about this approach. this workflow file is shared by all forks. so changing it to run on main only also make it harder for everyone to run tests on their forks. and since most contributors are using issue specific branches from their fork to raise PRs, this means basically no tests anywhere unless there's a PR into main.

gene very recently addressed some of the 'test flood' issue by introducing concurrency guards. so we should already be in better shape. i implore you to reconsider this drastic reduction in testing frequency.

@priyam0k priyam0k left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

looks fine

@priyam0k

priyam0k commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

since most contributors are using issue specific branches from their fork to raise PRs, this means basically no tests anywhere unless there's a PR into main.

missed this part, can we switch main to ** instead so ci still runs on feature branch pushes?

@kennethshsu

Copy link
Copy Markdown
Member Author

I guess I was thinking about conserving the CI resources here especially with that huge duplicator matrix when both push and PR run on the same commit.

means basically no tests anywhere unless there's a PR into main

This is not true? On line 9, CIs run when there's a PR open, even on PRs into branches that's not main, that's why you get those emails. This CI run on PR is unrestricted by branch names.

I think this might help:

Scenario push: '**' push: main
Push to main Yes Yes
Push to feature/foo (no PR) Yes No
PR into main Yes (twice from push + pull_request) Yes (once only)
PR into some other branch Yes (twice) Yes (once only)

By the way, I am not married to push: main here, but I think it's good practice to avoid running unnecessary tests. Anyways if you both feel strongly about the ** we can go with that!

@henrydingliu

Copy link
Copy Markdown
Member

yes, what we are saying is that committing to feature/foo is the predominant path that collaborators take. PR into some other branch rarely happens. i think pretty much only I've been using it to move from branches on my person fork to a branch on this repo.

@priyam0k

priyam0k commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

agreed with henry. let's go with **!

@kennethshsu

Copy link
Copy Markdown
Member Author

K let me close this we can go with #1183

@kennethshsu kennethshsu closed this Aug 3, 2026
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.

[BUG] unittest not triggered on branch names with slash

3 participants