GitHub Actions radicalized me
-
There are definitely ways to run partial testing suites on modified code only. I feel like much of what you're complaining about is an already solved problem.
It can be finicky to set up and mistakes can be made easily. Often you have to manually replicate the entire internal dependency tree of your project in the checks so that there are no false positive test results. There are some per-language solutions, and there's Nix which is almost built for this sort of thing, but both come with drawbacks as well.
-
There are definitely ways to run partial testing suites on modified code only. I feel like much of what you're complaining about is an already solved problem.
Yeah there are ways to run partial tests on modified code only. But they interact poorly with GH required checks. https://github.com/orgs/community/discussions/44490 goes into a lot more detail on similar problems people are having with GH actions - though our problem is with external CICD tools that report back to GH. Though it does look like they have updated the docs that are linked to in that discussion so maybe something has recently changed with GH actions - but I bet it still exists for external tooling.
-
The real problem is merging before waiting for that one slow CI pipeline to complete
One problem is GHs auto-merge when ready button. It will merge when there are still tests running unless they are required. It would be much better if the auto merges took into account all checks and not just required ones.
-
Eww, no. You're doing tests wrong. The point of tests is to understand whether changes to the code (or dependencies) break any functionality. If you have failing tests, it makes this task very difficult and time consuming for people who need it most, i.e. people new to the project. "Is this test failing because of something I've done? <half an hour of debugging later> Oh, it was broken before my changes too!". If you insist on adding broken tests, mark them as "expected to fail" at least, so that they don't affect the overall test suite result (and when someone fixes the functionality they have to un-mark them as expected to fail), and the checkmark is green. You should never merge PRs/MRs which fail any tests - it is an extremely bad habit and forms a bad culture in your project.
You're both right. You're both wrong.
- You write tests for functionality before you write the functionality.
- You code the functionality so the tests pass.
- Then, and only then, the test becomes a regression test and is enabled in your CI automation.
- If the test ever breaks again the merge is blocked.
If you only write tests after you've written the code then the test will test that the code does what the code does. Your brain is already polluted and you're not capable of writing a good test.
Having tests that fail is fine, as long as they're not part of your regression tests.
-
Eww, no. You're doing tests wrong. The point of tests is to understand whether changes to the code (or dependencies) break any functionality. If you have failing tests, it makes this task very difficult and time consuming for people who need it most, i.e. people new to the project. "Is this test failing because of something I've done? <half an hour of debugging later> Oh, it was broken before my changes too!". If you insist on adding broken tests, mark them as "expected to fail" at least, so that they don't affect the overall test suite result (and when someone fixes the functionality they have to un-mark them as expected to fail), and the checkmark is green. You should never merge PRs/MRs which fail any tests - it is an extremely bad habit and forms a bad culture in your project.
wrote on last edited by [email protected]There are two different things mentioned here, which I feel I need to clarify:
First, what you said about merging / creating a PR with broken tests. Absolutely you shouldn't do that, because you should only merge once the feature is finished. If a test doesn't work, then either it's testing for the wrong aspect and should be rewritten, or the functionality doesn't work 100% yet, so the feature isn't ready to get merged. Even if you're waiting for some other feature to get ready, because you need to integrate it or something, you're still waiting, so the feature isn't ready.
At the same time, the OP's point about tests being supposed to fail at first isn't too far off the mark either, because that's precisely how TDD works. If you're applying that philosophy (which I personally condone), then that's exactly what you do: Write the test first, checking for expected behaviour (which is taken from the specification), which will obviously fail, and only then write the code implementing that behaviour.
But, even then, that failing test should be contained to e.g. the feature branch you're working on, never going in a PR while it's still failing.
Once that feature has been merged, then yes, the test should never fail again, because that indicates a new change having sabotaged some area of that feature. Even if the new feature is considered "essential" or "high priority" while the old feature is not, ignoring the failure is one of the easiest ways to build up technical debt, so you should damn well fix that now.
-
There are two different things mentioned here, which I feel I need to clarify:
First, what you said about merging / creating a PR with broken tests. Absolutely you shouldn't do that, because you should only merge once the feature is finished. If a test doesn't work, then either it's testing for the wrong aspect and should be rewritten, or the functionality doesn't work 100% yet, so the feature isn't ready to get merged. Even if you're waiting for some other feature to get ready, because you need to integrate it or something, you're still waiting, so the feature isn't ready.
At the same time, the OP's point about tests being supposed to fail at first isn't too far off the mark either, because that's precisely how TDD works. If you're applying that philosophy (which I personally condone), then that's exactly what you do: Write the test first, checking for expected behaviour (which is taken from the specification), which will obviously fail, and only then write the code implementing that behaviour.
But, even then, that failing test should be contained to e.g. the feature branch you're working on, never going in a PR while it's still failing.
Once that feature has been merged, then yes, the test should never fail again, because that indicates a new change having sabotaged some area of that feature. Even if the new feature is considered "essential" or "high priority" while the old feature is not, ignoring the failure is one of the easiest ways to build up technical debt, so you should damn well fix that now.
wrote on last edited by [email protected]I concede that on a feature branch, before a PR is made, it's ok to have some failing tests, as long as the only tests failing are related to that feature. You should squash those commits after the feature is complete so that no commit has a failing test once it's on master.
(I'm also a fan of TDD, although for me it means Type-Driven Development, but I digress...)
-
You're both right. You're both wrong.
- You write tests for functionality before you write the functionality.
- You code the functionality so the tests pass.
- Then, and only then, the test becomes a regression test and is enabled in your CI automation.
- If the test ever breaks again the merge is blocked.
If you only write tests after you've written the code then the test will test that the code does what the code does. Your brain is already polluted and you're not capable of writing a good test.
Having tests that fail is fine, as long as they're not part of your regression tests.
wrote on last edited by [email protected]- You write tests for functionality before you write the functionality.
- You code the functionality so the tests pass.
- Then, and only then, the test becomes a regression test and is enabled in your CI automation.
- If the test ever breaks again the merge is blocked.
I disagree. Merging should be blocked on any failing test. No commit should be merged to master with a failing test. If you want to write tests first, then do that on a feature branch, but squash the commits properly before merging. Or add them as disabled first and enable after the feature is implemented. The enabled tests must always pass on every commit on master.
-
The real problem is merging before waiting for that one slow CI pipeline to complete
gitlab has a feature where you can set it to auto-merge when and if the CI completes successfully
-
- You write tests for functionality before you write the functionality.
- You code the functionality so the tests pass.
- Then, and only then, the test becomes a regression test and is enabled in your CI automation.
- If the test ever breaks again the merge is blocked.
I disagree. Merging should be blocked on any failing test. No commit should be merged to master with a failing test. If you want to write tests first, then do that on a feature branch, but squash the commits properly before merging. Or add them as disabled first and enable after the feature is implemented. The enabled tests must always pass on every commit on master.
So I can never commit a test without also implementing the functionality?
That's madness.
-
So I can never commit a test without also implementing the functionality?
That's madness.
You can, but not on master.
-
One problem is GHs auto-merge when ready button. It will merge when there are still tests running unless they are required. It would be much better if the auto merges took into account all checks and not just required ones.
It tests passing is a requirement of merging, then you should set the tests as required.
-
It tests passing is a requirement of merging, then you should set the tests as required.
If you have
folderAandfolderBeach with their own set of tests. You don't needfolderAs tests to run with a change tofolderB. Most CI/CD systems can do this easily enough with two different reports. But you cannot mark them both as required as they both wont always run. Instead you need a complicated fan out pipelines in your CICD system so you can only have one report back to GH or you need to always spawn a job for both folders and have the ones that dont need to run return successful. Neither of these is very good and becomes very complex when you are working with large monorepos.It would be much better if the CICD system that knows which pipelines it needs to run for a given PR could tell GH about which tests are required for a particular PR and if you could configure GH to wait for that report from the CICD system. Or at the very least if the auto-merge was blocked for any failed checks and the manual merge button was only blocked on required checks.
-
If you have
folderAandfolderBeach with their own set of tests. You don't needfolderAs tests to run with a change tofolderB. Most CI/CD systems can do this easily enough with two different reports. But you cannot mark them both as required as they both wont always run. Instead you need a complicated fan out pipelines in your CICD system so you can only have one report back to GH or you need to always spawn a job for both folders and have the ones that dont need to run return successful. Neither of these is very good and becomes very complex when you are working with large monorepos.It would be much better if the CICD system that knows which pipelines it needs to run for a given PR could tell GH about which tests are required for a particular PR and if you could configure GH to wait for that report from the CICD system. Or at the very least if the auto-merge was blocked for any failed checks and the manual merge button was only blocked on required checks.
You can have certain jobs run based on what directories or files were modified. If projectA was the only one modified, it can run just projectA's tests.
-
If you have
folderAandfolderBeach with their own set of tests. You don't needfolderAs tests to run with a change tofolderB. Most CI/CD systems can do this easily enough with two different reports. But you cannot mark them both as required as they both wont always run. Instead you need a complicated fan out pipelines in your CICD system so you can only have one report back to GH or you need to always spawn a job for both folders and have the ones that dont need to run return successful. Neither of these is very good and becomes very complex when you are working with large monorepos.It would be much better if the CICD system that knows which pipelines it needs to run for a given PR could tell GH about which tests are required for a particular PR and if you could configure GH to wait for that report from the CICD system. Or at the very least if the auto-merge was blocked for any failed checks and the manual merge button was only blocked on required checks.
Both GitHub Actions and GitLab CI let you specify filepath rules for triggering jobs.
-
It tests passing is a requirement of merging, then you should set the tests as required.
Exactly; the OP image is saying that there's no point to doing that.
-
You can have certain jobs run based on what directories or files were modified. If projectA was the only one modified, it can run just projectA's tests.
Yes. They can. But they do not mix well with required checks. From githubs own documentation:
If a workflow is skipped due to path filtering, branch filtering or a commit message, then checks associated with that workflow will remain in a "Pending" state. A pull request that requires those checks to be successful will be blocked from merging.
If, however, a job within a workflow is skipped due to a conditional, it will report its status as "Success". For more information, see Using conditions to control job execution.
So even with github actions you cannot mix a required check and path/branch or any filtering on a workflow as the jobs will hang forever and you will never be able to merge the branch in. You can do either or, but not both at once and for larger complex projects you tend to want to do both. But instead you need complex complex workflows or workflows that always start and instead do internal checks to detect if they need to actually run or not. And this is with github actions - it is worst for external CICD tooling.
-
wrote on last edited by [email protected]
"You don't have the hiring and firing power."
-- Kitty, Arrested Development
