Reduce the number of git clones in the test framework #1686
Conversation
|
To me this feels like it leaks too much of the app's internals into the integration tests. They shouldn't need to care about whether the request they just made enqueued some background jobs or not -- running them synchronously in tests is pretty common across all frameworks. If the problem we're trying to solve is not performing a git clone for tests that don't need them, would just adding |
|
Also can you shed more light on why we're focusing on reducing allocations when running tests? Is this causing problems somewhere that I'm not aware of? |
|
☔ The latest upstream changes (presumably #1699) made this pull request unmergeable. Please resolve the merge conflicts. |
These changes originated from my attempts to profile memory usage. While the tests in While trimming down the test harness, I was able to find and extract the improvements in #1692, #1682, and #1669.
I see that this seems to be coupling tests to the implementation details of individual endpoints, but I see it as each test orchestrating the set of high-level components that are needed to test a piece of the site's functionality. Of the 178 tests here, 144 of them don't require anything more than a "web" instance and the database. These could potentially live in the main library, as they just use the test harness to optionally initialize users/tokens and inject There are only 34 tests which use the test harness to initialize the playback server. Here the test are already coupled to the implementation because there needs to be a proxy recording in
Beyond remembering to initialize the background worker (and this mistake is detected and provides an error message explaining what to do), the individual tests don't necessarily need to be aware of this (as with
That might help a bit, but the test harness would still be initializing an upstream repository for every test, assuming that the worker might need to be run. Plus I think there are other advantages to moving this into the test harness as mentioned above. |
|
☔ The latest upstream changes (presumably #1677) made this pull request unmergeable. Please resolve the merge conflicts. |
The test framework spends a lot of I/O and allocations initializing and cloning git repositories, even though most tests do not enqueue a background job to modify the index. A `TestApp::full()` method is added which initializes the `TestApp` with an index, background job runner, and playback proxy. It is also now possible to enqueue multiple crate publishes before running a batch of background jobs. A `Drop` impl on `TestAppInner` ensures that all pending migrations are run and that no jobs have been queued unless `TestApp::full()` was used. Unlike the `enqueue_publish` helper the `yank` and `unyank` methods automatically run migrations out of convenience since otherwise the database is not updated. This can be changed in the future if a test cares about controlling exactly when the jobs are run. Finally, the initial commit for each remaining upstream index is now made directly against the bare repo, rather than doing a clone and push.
There is no reason to initialize this client when in playback mode.
The remaining 12 tests that call `app()` directly do not need a proxy.
This list was obtained by temporarily causing playback to panic if there was no `http-data` file when the proxy is started. The only remaining test that starts a proxy server which goes unused is `krate::yank_not_owner`, however this test does need an index. If more tests fall into this category over time we can switch to a full builder API for orchestrating a `TestApp` but that doesn't seem necessary yet.
jtgeibel
force-pushed
the
reduce-test-allocations
branch
from
20a7ded to
85ee13e
Compare
|
@bors: r+ |
|
📌 Commit 85ee13e has been approved by |
Reduce the number of git clones in the test framework The test framework spends a lot of I/O and allocations initializing and cloning git repositories, even though most tests do not enqueue a background job to modify the index. A `TestApp::full()` method is added which initializes the `TestApp` with an index, background job runner, and playback proxy. It is also now possible to enqueue multiple crate publishes before running a batch of background jobs. A `Drop` impl on `TestAppInner` ensures that all pending migrations are run and that no jobs have been queued unless `TestApp::full()` was used. Unlike the `enqueue_publish` helper the `yank` and `unyank` methods automatically run migrations out of convenience since otherwise the database is not updated. This can be changed in the future if a test cares about controlling exactly when the jobs are run. Finally, this PR includes several other improvements: * The initial commit for each remaining upstream index is now made directly against the bare repo, rather than doing a clone and push. * The proxy only initializes a `Client` when recording. * A playback proxy is not created when calling `app()` Overall, when running the tests in `all`, these changes reduce the cumulative allocation size from 3.45 GB to 689 MB. Allocation counts are reduced from 4,625,804 to 1,611,351.
|
☀️ Test successful - checks-travis |
The test framework spends a lot of I/O and allocations initializing and
cloning git repositories, even though most tests do not enqueue a
background job to modify the index. A
TestApp::full()method isadded which initializes the
TestAppwith an index, background jobrunner, and playback proxy.
It is also now possible to enqueue multiple crate publishes before
running a batch of background jobs. A
Dropimpl onTestAppInnerensures that all pending migrations are run and that no jobs have been
queued unless
TestApp::full()was used.Unlike the
enqueue_publishhelper theyankandunyankmethodsautomatically run migrations out of convenience since otherwise the
database is not updated. This can be changed in the future if a test
cares about controlling exactly when the jobs are run.
Finally, this PR includes several other improvements:
directly against the bare repo, rather than doing a clone and push.
Clientwhen recording.app()Overall, when running the tests in
all, these changes reduce thecumulative allocation size from 3.45 GB to 689 MB. Allocation counts
are reduced from 4,625,804 to 1,611,351.