Conversation
Before this change, the integration tests checked whether each remote could set modtimes on every `bisync` step (`checkPreReqs`, called from `runBisync`), not once per remote. On a remote that isn't `IsLocal`, each check sleeps for 1s twice (the Google Cloud Storage per-object rate limit workaround from be73a10), for each side. That added 2-4s to each of the ~110 `bisync` steps in each test, which on a fast remote was most of the run: on the integration test server, the `:memory:` tests took about 14 minutes. After this change, the result of the check is remembered for each remote, so it runs, and sleeps, once per remote per test run. The `:memory:` tests now take about 7s locally.
Before this change, the integration tests dropped Dropbox's "Too many requests or write operations" notice from the logs only in the format it had when the tests were written, which began with the error code (`too_many_requests/...`). Since 110bf46 (v1.65), the notice begins with `Error `, so it was left in the log and caused a miscompare whenever Dropbox rate limited a run. After this change, the notice is dropped in either format.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this change do?
bisync: speed up integration tests on non-local remotes
Before this change, the bisync integration tests checked whether each remote could set modtimes on every
bisyncstep (checkPreReqs, called fromrunBisync), not once per remote. On a remote that isn'tIsLocal, each check sleeps for 1s twice (the Google Cloud Storage per-object rate limit workaround from be73a10), for each side, on top of the check's own API calls. That added 2-4s to each of the ~110bisyncsteps in each test.After this change, the result of the check is remembered for each remote, so it runs, and sleeps, once per remote per test run. The sleeps themselves are unchanged.
bisync: fix Dropbox rate limit notices failing the integration tests
The tests already drop Dropbox's "Too many requests or write operations" notice from the logs, but only in the format it had when the tests were written. Since 110bf46 (v1.65), the notice begins with
Error, so the filter stopped matching. Without the sleeps above, the tests run faster and are more likely to be rate limited. This change widens the pattern to match either format.Benchmarks
Last night's integration test logs, next to runs with this PR:
TestSFTPRclone:TestDrive:TestBisyncRemoteRemotecases it didn't reach ("not enough time to start test") needed a try 2, ~87 min in total (try 1, try 2)TestDropbox::memory:The "with this PR" runs were on my machine, so only the SFTP row is a like-for-like comparison: master took 1007s there, against 66s with this PR. For Drive and Dropbox, some of the difference is network and account, not this change.
The Dropbox run had four failures, none caused by this change:
Those four cases passed on a rerun, both with this PR and on master.
Testing
go test ./cmd/bisync -race(local): passes-remote :memory:: 6.8s-remote TestCrypt:: 24.7sTestSFTPRclone:,TestDrive:,TestDropbox:: as aboveLinked issue
No issue -- I found this while measuring bisync test coverage against more remotes.
Checklist
test_allpasses for this backend and if submitting a new backend can provide a test account for the integration tester - see CONTRIBUTING.md.