4 ms·

Yeah, this is how I’ve worked for 16 years. The unit tests always pass. Maybe once or twice a year you can go way out on a limb and do something exploratory whe

Yeah, this is how I’ve worked for 16 years. The unit tests always pass. Maybe once or twice a year you can go way out on a limb and do something exploratory where you don’t care about the tests passing for every commit, but... part of TDD and CI is that the tests always pass. In some ways, I think the feature branch->PR model of today is a step backwards from traditional central server single-trunk development of years gone by (CVS/Perforce/SVN). Branches and PRs work better for F/OSS development, and for monster refactoring, but I kinda long for the days of a normal team just committing to master/trunk several times a day.

jrockway 8y ago
I think PRs are logically the same as Perforce changelists. The fact that you can pick apart a PR into multiple commits does pollute the history when you merge, though, unless your tools treat the PR as a single atomic commit (for bisection, etc.) I have never treated the individual commits inside a PR as anything other than "I'd like to backup my workstation to the cloud now", but if people are treating them as something meaningful, I think that's a mistake.
- XorNot 8y ago
  The popularity of rebase merging in Git I think is partly to blame here. Rebase merging tries to make everything seem like real history, but no one ever seems to have a solid idea of what that actually is supposed to mean (and CI systems don't make it easy to configure "test all commits" or declare what you want your commit history to mean).
  - jrockway 8y ago
    Yup. I always picked "merge" instead of "rebase" in Gerrit because it's not like I tested the result of applying each patch to the master branch. All I did was decide that mine worked and that I wanted to merge my branch with the upstream. People like using clever features to make the history inaccurate, which seems like a bad idea to me. (It's "cleaner", but wrong.)
    - int0x80 8y ago
      If you internally rebase to cleanup history you have to test again. Ignoring that step is wrong. Local rebase can be good and useful.
  - Vinnl 8y ago
    There's no such thing as "real history" - even without rebasing, your commits do not reflect the development of the code; they reflect practically arbitrary moments in time at which you chose to commit. The idea behind rebasing is not to make it seem like "real history", but to have all changes in a commit to be related. They're not a reflection of how you built the code, but how you can understand it. They're documentation, and as such, you spend effort on them.
- int0x80 8y ago
  I don't know if I'm reading your comment correctly, sorry if I'm not... but individual commits within a PR are absolutely not a backup. Every commit in the PR has to be a logical unit, meaningful, atomic and stand on its own. The PR, as a high level feature, is just a grouping. But single commits have to be useful and be a history/build up of how high level features are being developed. This granularity is important to logically isolate the changes and make them bisectable among other things.
ewjordan 8y ago
Once your test suite is big enough that it won't run within a minute or two, it's kind of hard to keep a "green on each commit" workflow, though. Especially with UI and visual diff tests in the mix, I certainly don't want to tie up my local computer for a full test run when I'm just fixing a small bug; pushing the commits in a branch and letting the full suite run against that while I move on to another task is a much better use of resources, and that's where the PR model seems most useful to me. Are there workflows that avoid this problem but still enforce every commit being green?
- closeparen 8y ago
  In Phabricator you make your changes on a local branch, running "arc diff" to post or update a changeset. CI runs on each update to the changeset. You're expected to have CI passing before you "arc land" (squash merge to master and push), and usually before you solicit code review. Commit whatever garbage you want locally and even publish it to the Diff, but your code doesn't touch master in the central repository until reviewed and passing.
- jonstewart 8y ago
  Sure, I separate heavyweight tests out into integration/acceptance tests (the side effects make them slow; the side effects mean they're not really unit tests). So, I don't commit until the unit tests pass, and then a PR triggers heavyweight tests on my build server, to be sure you're clean before the merge to master. But if there's too much divergence between unit test and integration test pass/fail rates, then unit test coverage needs to be improved; obviously, easier said then done when you have old pre-TDD that's tightly coupled and/or code that's interacting with the system and incurring side effects that are hard/expensive to test.

jrockway 8y ago

I think PRs are logically the same as Perforce changelists. The fact that you can pick apart a PR into multiple commits does pollute the history when you merge, though, unless your tools treat the PR as a single atomic commit (for bisection, etc.) I have never treated the individual commits inside a PR as anything other than "I'd like to backup my workstation to the cloud now", but if people are treating them as something meaningful, I think that's a mistake.

XorNot 8y ago
The popularity of rebase merging in Git I think is partly to blame here. Rebase merging tries to make everything seem like real history, but no one ever seems to have a solid idea of what that actually is supposed to mean (and CI systems don't make it easy to configure "test all commits" or declare what you want your commit history to mean).
- jrockway 8y ago
  Yup. I always picked "merge" instead of "rebase" in Gerrit because it's not like I tested the result of applying each patch to the master branch. All I did was decide that mine worked and that I wanted to merge my branch with the upstream. People like using clever features to make the history inaccurate, which seems like a bad idea to me. (It's "cleaner", but wrong.)
  - int0x80 8y ago
    If you internally rebase to cleanup history you have to test again. Ignoring that step is wrong. Local rebase can be good and useful.
- Vinnl 8y ago
  There's no such thing as "real history" - even without rebasing, your commits do not reflect the development of the code; they reflect practically arbitrary moments in time at which you chose to commit. The idea behind rebasing is not to make it seem like "real history", but to have all changes in a commit to be related. They're not a reflection of how you built the code, but how you can understand it. They're documentation, and as such, you spend effort on them.
int0x80 8y ago
I don't know if I'm reading your comment correctly, sorry if I'm not... but individual commits within a PR are absolutely not a backup. Every commit in the PR has to be a logical unit, meaningful, atomic and stand on its own. The PR, as a high level feature, is just a grouping. But single commits have to be useful and be a history/build up of how high level features are being developed. This granularity is important to logically isolate the changes and make them bisectable among other things.

ewjordan 8y ago

Once your test suite is big enough that it won't run within a minute or two, it's kind of hard to keep a "green on each commit" workflow, though. Especially with UI and visual diff tests in the mix, I certainly don't want to tie up my local computer for a full test run when I'm just fixing a small bug; pushing the commits in a branch and letting the full suite run against that while I move on to another task is a much better use of resources, and that's where the PR model seems most useful to me. Are there workflows that avoid this problem but still enforce every commit being green?

closeparen 8y ago
In Phabricator you make your changes on a local branch, running "arc diff" to post or update a changeset. CI runs on each update to the changeset. You're expected to have CI passing before you "arc land" (squash merge to master and push), and usually before you solicit code review. Commit whatever garbage you want locally and even publish it to the Diff, but your code doesn't touch master in the central repository until reviewed and passing.
jonstewart 8y ago
Sure, I separate heavyweight tests out into integration/acceptance tests (the side effects make them slow; the side effects mean they're not really unit tests). So, I don't commit until the unit tests pass, and then a PR triggers heavyweight tests on my build server, to be sure you're clean before the merge to master. But if there's too much divergence between unit test and integration test pass/fail rates, then unit test coverage needs to be improved; obviously, easier said then done when you have old pre-TDD that's tightly coupled and/or code that's interacting with the system and incurring side effects that are hard/expensive to test.