4 ms·

I presume merge queues are useless if you have flaky tests?

jakub_g 3y ago
A good MQ impl allows you to make the MQ checks different than in PR. And you choose for MQ builds only a subset of tests which are known to be non-flaky. Merge queue builds should be fast and rock stable, following the 80/20 rule.
mfenniak 3y ago
They're super annoying if you have flaky tests. If the tests are such that you can automate retrying and get to a reasonably low false-positive rate, it can be usable.
- fosterfriends 3y ago
  I like adopting a merge queue as a forcing function to deal with flaky tests.
  - henryfjordan 3y ago
    Why jump to a merge queue? Github has had a feature to grey-out the merge button if the tests aren't passing for years.
- ankitdce 3y ago
  Excited to see this thread. Aviator also offers a high performance MergeQueue with flaky test support: https://docs.aviator.co/mergequeue/managing-flaky-tests-in-mergequeue https://docs.aviator.co/mergequeue/managing-flaky-tests-in-m... Disclaimer: I'm the co-founder of Aviator
masklinn 3y ago
Not useless, you get sprurious rejections and have to resubmit, like running CI. and you can retry—on-failure to increase the odds. Some test frameworks support rerunning flaky tests. They’re a good justification to work on that tho.
wizofaus 3y ago
From what I've observed semantic merge issues always get picked up at compile time - hence only the compile phase need pass to allow a queued merge through, but I guess if you're working a lot with weakly-typed or interpreted languages it'd be different. It's hard not to think there should be some way of automatically flagging likely issues where, say, commits in one PR refer to particular identifiers that were modified in parallel PRs, but it definitely goes well beyond what diff-merge tools typically do today.
- skissane 3y ago
  > From what I've observed semantic merge issues always get picked up at compile time - hence only the compile phase need pass to allow a queued merge through, but I guess if you're working a lot with weakly-typed or interpreted languages it'd be different. I agree that 90% of the time, with strongly typed languages, a compile is enough. But even in those languages, you can still get semantic merge issues which aren’t compilation-related. Suppose your app has a config file. The first PR renames one of the configuration keys in the config file, the second PR adds some new code which reads that configuration key. The combination compiles fine, but the second PR fails in integration tests, because the config key it was expecting wasn’t there any more. Of course this is a sign of a design flaw - the name of the config key should occur only once in the program (say as a constant), so both PRs would use the constant, and the second PR would get the change in constant value from the first, or else the first renames the constant so the second doesn’t compile. Or even a higher-level API which deals with the semantics of what the config represents instead of just key-value strings. But a lot of large legacy code bases contain design flaws like this, and fixing them can be a lot of work, so they don’t get fixed, at least not any time soon.
  - wizofaus 3y ago
    Sure, there are always going to be occasions where a combination of two bits of work done separately cause unexpected behaviour changes - but generally if there are integration/automated E2E tests that are capable of picking that sort of thing up I'd expect they typically get triggered after the mainline merge anyway. OTOH, I can understand at least wanting to ensure that it's not possible for two consecutive merges to the mainline branch to cause it stop building.

A good MQ impl allows you to make the MQ checks different than in PR. And you choose for MQ builds only a subset of tests which are known to be non-flaky. Merge queue builds should be fast and rock stable, following the 80/20 rule.

mfenniak 3y ago

They're super annoying if you have flaky tests. If the tests are such that you can automate retrying and get to a reasonably low false-positive rate, it can be usable.

fosterfriends 3y ago
I like adopting a merge queue as a forcing function to deal with flaky tests.
- henryfjordan 3y ago
  Why jump to a merge queue? Github has had a feature to grey-out the merge button if the tests aren't passing for years.
ankitdce 3y ago
Excited to see this thread. Aviator also offers a high performance MergeQueue with flaky test support: https://docs.aviator.co/mergequeue/managing-flaky-tests-in-mergequeue https://docs.aviator.co/mergequeue/managing-flaky-tests-in-m... Disclaimer: I'm the co-founder of Aviator

masklinn 3y ago

Not useless, you get sprurious rejections and have to resubmit, like running CI. and you can retry—on-failure to increase the odds. Some test frameworks support rerunning flaky tests. They’re a good justification to work on that tho.

wizofaus 3y ago

From what I've observed semantic merge issues always get picked up at compile time - hence only the compile phase need pass to allow a queued merge through, but I guess if you're working a lot with weakly-typed or interpreted languages it'd be different. It's hard not to think there should be some way of automatically flagging likely issues where, say, commits in one PR refer to particular identifiers that were modified in parallel PRs, but it definitely goes well beyond what diff-merge tools typically do today.

skissane 3y ago
> From what I've observed semantic merge issues always get picked up at compile time - hence only the compile phase need pass to allow a queued merge through, but I guess if you're working a lot with weakly-typed or interpreted languages it'd be different. I agree that 90% of the time, with strongly typed languages, a compile is enough. But even in those languages, you can still get semantic merge issues which aren’t compilation-related. Suppose your app has a config file. The first PR renames one of the configuration keys in the config file, the second PR adds some new code which reads that configuration key. The combination compiles fine, but the second PR fails in integration tests, because the config key it was expecting wasn’t there any more. Of course this is a sign of a design flaw - the name of the config key should occur only once in the program (say as a constant), so both PRs would use the constant, and the second PR would get the change in constant value from the first, or else the first renames the constant so the second doesn’t compile. Or even a higher-level API which deals with the semantics of what the config represents instead of just key-value strings. But a lot of large legacy code bases contain design flaws like this, and fixing them can be a lot of work, so they don’t get fixed, at least not any time soon.
- wizofaus 3y ago
  Sure, there are always going to be occasions where a combination of two bits of work done separately cause unexpected behaviour changes - but generally if there are integration/automated E2E tests that are capable of picking that sort of thing up I'd expect they typically get triggered after the mainline merge anyway. OTOH, I can understand at least wanting to ensure that it's not possible for two consecutive merges to the mainline branch to cause it stop building.