Retry LSP tests in case of error #5342
Conversation
|
Seems like a good compromise, as much as it's an OR of tests and not an AND that would detect instability |
|
I think we should revert this change, because it makes it easier for unstable tests to be merged, which is really bad. What we were already doing was running the IDE tests twice, which makes it more likely that a build will fail if there are unstable tests. This PR achieves the opposite, of making a failure less likely when there are unstable tests. Doing both at the same time, as we are now, is interesting :D
I think doing it by hand is a mistake. We should fix the instability. I think it's very little work and there is no reason not to put it at the top of the priorities. The reason we haven't done it I think is because it's too easy to rerun the tests. I think ideally we would run the IDE tests not twice but a 100 times, so retrying becomes ineffective. |
We're not doing both at the same time. This PR removes the double execution.
I entirely agree that we should fix the instability, and that it should be high priority. I'm not as confident that it's easy, however. Once we think we've fixed it, I think we should remove this retry mechanism. If it's actually very easy, that will be soon. :) |
|
Okay.. well I still think we should revert this. With this PR we are much more at risk of introducing instabilities, which not only impacts development speed but can only be customer affecting bugs. |
This reverts commit 44ca1b2.
This reverts commit 44ca1b2. ### Description The recently merged PR #5342 argues that we should automatically retry failing IDE tests, because that's what we manually do anyways. However, we only do that in some cases. Specifically, when any of us make changes that may create unstable tests, such as changes in the IDE, then failing IDE tests should not be retried, manually or automatically. PR5342 makes it so that such a retry happens automatically, thereby making it much easier to introduce non-deterministic bugs either in IDE code or tests. That's why this PR is reverting that one. IMO there is little difference between a test that fails 100% of the time and one that fails <100% of the time. Retrying a failing test that fails <100% of the time is similar to saying "I believe my PR is not at fault for this test failing, please ignore it". A similar situation occurs when a PR gets merged that causes nightly to fail. However then we don't have a way for new PRs to say "please ignore nightly failing", and we explicitly block all PR until nightly is fixed again. A similar approach to unstable tests would be to not to retry failing builds until the instability is resolved, but instead to wait until an instability fix PR is merged. ### How has this been tested? It's a revert. <small>By submitting this pull request, I confirm that my contribution is made under the terms of the [MIT license](https://github.com/dafny-lang/dafny/blob/master/LICENSE.txt).</small>
Description
This PR changes how the language server unit tests execute, retrying them up to three times if they fail. Ultimately, this is what we're doing by hand, anyway, so we might as well automate it.
The proper solution to this problem is to ensure that the language server tests don't spuriously fail, but this will accelerate our development process in the meantime.
How has this been tested?
By running CI normally.
By submitting this pull request, I confirm that my contribution is made under the terms of the MIT license.