fix: do not lost "testXReqId" when using "parallelLimit"

gemini-testing · Jan 30, 2024 · 7f83de7 · 7f83de7
1 parent 76f8c69
commit 7f83de7
Show file tree

Hide file tree

Showing 3 changed files with 32 additions and 11 deletions.
diff --git a/src/browser-pool/limited-pool.js b/src/browser-pool/limited-pool.js
@@ -49,7 +49,8 @@ module.exports = class LimitedPool extends Pool {
         --this._requests;
 
         const nextRequest = this._lookAtNextRequest();
-        const compositeIdForNextRequest = nextRequest && buildCompositeBrowserId(nextRequest.id, nextRequest.version);
+        const compositeIdForNextRequest =
+            nextRequest && buildCompositeBrowserId(nextRequest.id, nextRequest.opts.version);
         const hasFreeSlots = this._launched < this._limit;
         const shouldFreeUnusedResource = this._isSpecificBrowserLimiter && this._launched > this._requests;
         const force = opts.force || shouldFreeUnusedResource;
@@ -83,10 +84,9 @@ module.exports = class LimitedPool extends Pool {
         this.log("queuing the request");
 
         const queue = opts.highPriority ? this._highPriorityRequestQueue : this._requestQueue;
-        const { version } = opts;
 
         return new Promise((resolve, reject) => {
-            queue.push({ id, version, resolve, reject });
+            queue.push({ id, opts, resolve, reject });
         });
     }
 
@@ -111,11 +111,12 @@ module.exports = class LimitedPool extends Pool {
         const queued = this._highPriorityRequestQueue.shift() || this._requestQueue.shift();
 
         if (queued) {
-            const compositeId = buildCompositeBrowserId(queued.id, queued.version);
+            const compositeId = buildCompositeBrowserId(queued.id, queued.opts.version);
 
             this.log(`has queued requests for ${compositeId}`);
             this.log(`remaining queue length: ${this._requestQueue.length}`);
-            this._newBrowser(queued.id, { version: queued.version }).then(queued.resolve, queued.reject);
+
+            this._newBrowser(queued.id, queued.opts).then(queued.resolve, queued.reject);
         } else {
             this._launched--;
         }

diff --git a/src/runner/test-runner/regular-test-runner.js b/src/runner/test-runner/regular-test-runner.js
@@ -86,7 +86,7 @@ module.exports = class RegularTestRunner extends Runner {
 
             this._browser = await this._browserAgent.getBrowser({ state });
 
-            // use correct state for cached browsers
+            // TODO: move logic to caching pool (in order to use correct state for cached browsers)
             if (this._browser.state.testXReqId !== state.testXReqId) {
                 this._browser.applyState(state);
             }

diff --git a/test/src/browser-pool/limited-pool.js b/test/src/browser-pool/limited-pool.js
@@ -32,13 +32,33 @@ describe("browser-pool/limited-pool", () => {
             assert.equal(bro, browser);
         });
 
-        it("should pass opts to underlying pool", async () => {
-            const browser = stubBrowser("bro");
-            underlyingPool.getBrowser.returns(Promise.resolve(browser));
+        describe("should pass opts to underlying pool from", () => {
+            it("first requested browser", async () => {
+                const browser = stubBrowser("bro");
+                underlyingPool.getBrowser.returns(Promise.resolve(browser));
 
-            await makePool_().getBrowser("bro", { some: "opt" });
+                await makePool_().getBrowser("bro", { some: "opt" });
 
-            assert.calledOnceWith(underlyingPool.getBrowser, "bro", { some: "opt" });
+                assert.calledOnceWith(underlyingPool.getBrowser, "bro", { some: "opt" });
+            });
+
+            it("queued browser", async () => {
+                const browser1 = stubBrowser("bro");
+                const browser2 = stubBrowser("bro");
+                underlyingPool.getBrowser
+                    .onFirstCall()
+                    .returns(Promise.resolve(browser1))
+                    .onSecondCall()
+                    .returns(Promise.resolve(browser2));
+
+                const pool = await makePool_({ limit: 1 });
+                await pool.getBrowser("bro", { some: "opt1" });
+                // should be called without await
+                Promise.delay(100).then(() => pool.freeBrowser(browser1));
+                await pool.getBrowser("bro", { another: "opt2" });
+
+                assert.calledWith(underlyingPool.getBrowser.secondCall, "bro", { another: "opt2" });
+            });
         });
     });