Run integration tests nightly rather than on every pull request - #1137
Run integration tests nightly rather than on every pull request#1137aalhossary wants to merge 1 commit into
Conversation
The integration tests reach CATH, ECOD, RCSB, EBI and others. That is what makes them worth having: they are how we learn that an upstream service changed a URL, a format or a redirect. It is also what makes them a poor gate on pull requests, because any of those services having a bad day blocks every contributor, and a real regression then cannot be distinguished from the surrounding noise. That is not hypothetical. Every PR Build since 2025-12-19 has failed, on all five matrix jobs, whatever the pull request contained - a one-line dependency bump fails exactly as a feature branch does. Two integration tests account for it: CathDomainTest, because download.cathdb.info began redirecting http to https, and EcodInstallationTest.testVersion, which downloads a 657 MB file from a slow server once per matrix job. Master Build already excludes the module with -pl '!biojava-integrationtest'. This applies the same exclusion to pull requests and adds a scheduled workflow that runs the full suite nightly, so the coverage is kept but is no longer in anybody's way. An upstream break still gets caught, within a day, by a run whose failure means what it says. The nightly job uses a single JDK, since the pull request build already covers 11, 17 and 21 and the point here is the network paths. It carries a 90 minute timeout, because these downloads are large and the six hour default is not a useful ceiling, and it keeps surefire reports for 30 days on failure, since GitHub expires run logs after 90 and those reports are what identify which service misbehaved.
On reversing a deliberate choiceWorth putting the history on the table, since this PR undoes something that was decided on purpose rather than by accident. #910 raised network-dependent tests as a flakiness risk in 2020. Closing it in 2023, @josemduarte wrote:
So integration tests were deliberately placed on pull requests, and deliberately kept off master. This PR takes them off the only place they currently run. That deserves an argument rather than a shrug. The argument is that the premise has stopped holding. Running them on pull requests is sound for as long as a red check carries information. Since 2025-12-19 it has not: every #1022 is also relevant. It tried to solve this from the other end, with a JUnit rule that skips tests when a network exception occurs. It stayed open for nine months and was closed unmerged. That is not a criticism of the approach — it suggests making network tests reliable in place is harder than changing when they run. What this keeps is the coverage @josemduarte asked for. The tests still run, nightly, against the same services, and an upstream change is still caught within a day — by a run whose failure names the service and is not competing for attention with unrelated pull requests. #1135 exists because that signal did eventually get through; it just took eight months and a manual investigation rather than one nightly failure. What this gives up, plainly: a pull request that breaks an integration test is not caught until that night. For changes touching the download or parsing paths, that is a real regression window. If that trade is unacceptable, the coherent alternative is to fix the two tests instead and close this — and that alternative has since been carried out: #1133 fixes CATH and #1141 fixes ECOD, so the build can be green with the integration tests still running. I would rather you reject this deliberately than merge it by default. Edited: ECOD turned out to be a bug rather than something needing a decision — the distribution format changed (#1139), fixed by #1141. Worth noting that it is also the best evidence for this PR: a nightly run would have named it the next morning instead of it hiding for months inside an unrelated red check. |
Addresses the structural half of #1135.
The integration tests reach CATH, ECOD, RCSB, EBI and others. That is what makes them worth having — they are how we learn that an upstream service changed a URL, a format or a redirect. It is also what makes them a poor gate on pull requests, because any of those services having a bad day blocks every contributor, and a real regression then cannot be distinguished from the surrounding noise.
That is not hypothetical. Every
PR Buildsince 2025-12-19 has failed, on all five matrix jobs, whatever the pull request contained — a one-line dependency bump (#1132) fails exactly as a feature branch does. Two integration tests account for it:CathDomainTest, becausedownload.cathdb.infobegan redirecting http to https (#1138), andEcodInstallationTest.testVersion, because ECOD redesigned its distribution format (#1139).Master Buildalready excludes the module with-pl '!biojava-integrationtest'. This applies the same exclusion to pull requests and adds a scheduled workflow that runs the full suite nightly, so the coverage is kept but is no longer in anybody's way. An upstream break still gets caught, within a day, by a run whose failure means what it says.Both failures are now fixed, which changes the argument for this PR
#1133 fixes CATH and #1141 fixes ECOD, so with those merged
PR Buildshould be green with the integration tests still running. The emergency case for this PR is therefore gone, and I would rather say so than let it merge on momentum.What is left is the stronger argument, and it is the one worth weighing:
EcodInstallationTest.testVersion:285 Values should be different. Actual: latest, on somebody else's unrelated pull request, competing for attention with a second failure and with eight months of identical red checks. It named neither ECOD nor the format. A nightly run would have reported it the following morning, alone, against a tree nobody had just changed.ecod.latest.domains.txtis 657 MB, pulled once per matrix job, five times per pull request. That is not why the test failed — see ECOD changed its distribution format: neither the version header nor the column layout is recognised #1139 — but it is real, and it buys coverage that a one-second offline test now provides better (Support the new ECOD distribution format #1141 adds fixtures pinning every ECOD file layout the project has ever read).So the trade is not "keep the build green" any more. It is: should a pull request's check depend on five third-party services being up and fast, when the thing those services actually tell us arrives more usefully as a nightly failure?
The nightly job
workflow_dispatch, so it can be run by hand to confirm a service is back.What this gives up
Plainly: a pull request that breaks an integration test is not caught until that night. For changes touching the download or parsing paths, that is a real regression window. If that trade is unacceptable — and now that both tests are fixed it is a much more open question than when I opened this — the coherent response is to close this PR. I would rather that than have it merged by default.