From 088b5ee4eb64dca3bc2c01466916c2e637624683 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Thu, 10 Oct 2024 14:13:15 -0400 Subject: [PATCH 1/7] fix stuff..... --- .github/workflows/test-all.yml | 5 +++-- .../dataconnect-test-runner.ts | 3 ++- .../emulators/dataconnect-emulator.ts | 2 -- .../emulator-testing/emulators/emulator.ts | 20 ++++++++++++------- 4 files changed, 18 insertions(+), 12 deletions(-) diff --git a/.github/workflows/test-all.yml b/.github/workflows/test-all.yml index eda081d1df..3928612f6b 100644 --- a/.github/workflows/test-all.yml +++ b/.github/workflows/test-all.yml @@ -14,7 +14,7 @@ name: Test All Packages -on: pull_request +on: push env: # make chromedriver detect installed Chrome version and download the corresponding driver @@ -92,7 +92,8 @@ jobs: - name: Run unit tests # Ignore auth and firestore since they're handled in their own separate jobs. run: | - xvfb-run yarn lerna run --ignore '{firebase-messaging-integration-test,@firebase/auth*,@firebase/firestore*,firebase-firestore-integration-test}' test:ci + # xvfb-run yarn lerna run --ignore '{firebase-messaging-integration-test,@firebase/auth*,@firebase/firestore*,firebase-firestore-integration-test}' test:ci + xvfb-run yarn lerna run --scope @firebase/data-connect test node scripts/print_test_logs.js env: FIREBASE_TOKEN: ${{ secrets.FIREBASE_CLI_TOKEN }} diff --git a/scripts/emulator-testing/dataconnect-test-runner.ts b/scripts/emulator-testing/dataconnect-test-runner.ts index e362ef59cb..d68dbb85f0 100644 --- a/scripts/emulator-testing/dataconnect-test-runner.ts +++ b/scripts/emulator-testing/dataconnect-test-runner.ts @@ -18,6 +18,7 @@ import { DataConnectEmulator } from './emulators/dataconnect-emulator'; import { spawn } from 'child-process-promise'; import * as path from 'path'; + function runTest(port: number) { console.log( 'path: ' + path.resolve(__dirname, '../../packages/data-connect') @@ -38,7 +39,7 @@ async function run(): Promise { await emulator.setUp(); await runTest(emulator.port); } finally { - await emulator.tearDown(); + emulator.tearDown(); } } run().catch(err => { diff --git a/scripts/emulator-testing/emulators/dataconnect-emulator.ts b/scripts/emulator-testing/emulators/dataconnect-emulator.ts index efe5bdbe52..d69677f1d1 100644 --- a/scripts/emulator-testing/emulators/dataconnect-emulator.ts +++ b/scripts/emulator-testing/emulators/dataconnect-emulator.ts @@ -21,8 +21,6 @@ import { Emulator } from './emulator'; const DATABASE_EMULATOR_VERSION = '1.3.7'; export class DataConnectEmulator extends Emulator { - // namespace: string; - constructor(port = 3628) { const os = platform(); let urlString = ''; diff --git a/scripts/emulator-testing/emulators/emulator.ts b/scripts/emulator-testing/emulators/emulator.ts index 85f190bcba..d19b9c786f 100644 --- a/scripts/emulator-testing/emulators/emulator.ts +++ b/scripts/emulator-testing/emulators/emulator.ts @@ -97,7 +97,10 @@ export abstract class Emulator { // The execute permission is required for it to be able to start // with 'java -jar'. fs.chmod(filepath, 0o755, err => { - if (err) reject(err); + if (err) { + reject(err); + } + console.log(`Changed emulator file permissions to 'rwxr-xr-x'.`); this.binaryPath = filepath; if (this.copyToCache()) { @@ -121,11 +124,14 @@ export abstract class Emulator { } let promise: ChildProcessPromise; if (this.isDataConnect) { - promise = spawn(this.binaryPath, [ - 'dev', - '--local_connection_string', - "'postgresql://postgres:secretpassword@localhost:5432/postgres?sslmode=disable'" - ]); + promise = spawn( + this.binaryPath, + [ + 'dev', + '--local_connection_string', + "'postgresql://postgres:secretpassword@localhost:5432/postgres?sslmode=disable'" + ] + ); } else { promise = spawn( 'java', @@ -185,7 +191,7 @@ export abstract class Emulator { if (this.binaryPath) { console.log(`Deleting the emulator jar at ${this.binaryPath}`); - fs.unlinkSync(this.binaryPath); + // fs.unlinkSync(this.binaryPath); } } From bf93c4c63d3f950ff08a8b9edf0df7bb25af60ae Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Thu, 10 Oct 2024 14:39:13 -0400 Subject: [PATCH 2/7] try again --- scripts/emulator-testing/emulators/emulator.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/scripts/emulator-testing/emulators/emulator.ts b/scripts/emulator-testing/emulators/emulator.ts index d19b9c786f..df5815de61 100644 --- a/scripts/emulator-testing/emulators/emulator.ts +++ b/scripts/emulator-testing/emulators/emulator.ts @@ -58,12 +58,13 @@ export abstract class Emulator { if (err) reject(err); console.log(`Created temporary directory at [${dir}].`); const filepath: string = path.resolve(dir, this.binaryName); - const writer = fs.createWriteStream(filepath); + const buf: any[] = []; console.log(`Downloading emulator from [${this.binaryUrl}] ...`); // Map the DOM's fetch Reader to node's streaming file system // operations. We will need to access class members `binaryPath` and `copyToCache` after the // download completes. It's a compilation error to pass `this` into the named function // `readChunk`, so the download operation is wrapped in a promise that we wait upon. + console.log(process.memoryUsage().heapTotal); const downloadPromise = new Promise( (downloadComplete, downloadFailed) => { fetch(this.binaryUrl) @@ -75,9 +76,10 @@ export abstract class Emulator { const reader = resp.body.getReader(); reader.read().then(function readChunk({ done, value }): any { if (done) { + console.log('done download. buffer length:', buf.length); downloadComplete(); } else { - writer.write(value); + buf.push(...value); return reader.read().then(readChunk); } }); @@ -96,6 +98,8 @@ export abstract class Emulator { // Change emulator binary file permission to 'rwxr-xr-x'. // The execute permission is required for it to be able to start // with 'java -jar'. + fs.writeFileSync(filepath, new Uint8Array(buf)); + fs.chmod(filepath, 0o755, err => { if (err) { reject(err); @@ -190,8 +194,8 @@ export abstract class Emulator { } if (this.binaryPath) { - console.log(`Deleting the emulator jar at ${this.binaryPath}`); - // fs.unlinkSync(this.binaryPath); + console.log(`Deleting the emulator at ${this.binaryPath}`); + fs.unlinkSync(this.binaryPath); } } From 7153bb7ec8f0e165fc95a164aa1ce742302fd8e2 Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Fri, 11 Oct 2024 16:52:54 -0400 Subject: [PATCH 3/7] Used fixed size byte array --- .../emulator-testing/emulators/emulator.ts | 120 ++++++++---------- 1 file changed, 54 insertions(+), 66 deletions(-) diff --git a/scripts/emulator-testing/emulators/emulator.ts b/scripts/emulator-testing/emulators/emulator.ts index df5815de61..4688ab615e 100644 --- a/scripts/emulator-testing/emulators/emulator.ts +++ b/scripts/emulator-testing/emulators/emulator.ts @@ -53,70 +53,61 @@ export abstract class Emulator { return Promise.resolve(); } + const { name: tempDir } = tmp.dirSync({ unsafeCleanup: true }); + const filepath = path.resolve(tempDir, this.binaryName); return new Promise((resolve, reject) => { - tmp.dir((err: Error | null, dir: string) => { - if (err) reject(err); - console.log(`Created temporary directory at [${dir}].`); - const filepath: string = path.resolve(dir, this.binaryName); - const buf: any[] = []; - console.log(`Downloading emulator from [${this.binaryUrl}] ...`); - // Map the DOM's fetch Reader to node's streaming file system - // operations. We will need to access class members `binaryPath` and `copyToCache` after the - // download completes. It's a compilation error to pass `this` into the named function - // `readChunk`, so the download operation is wrapped in a promise that we wait upon. - console.log(process.memoryUsage().heapTotal); - const downloadPromise = new Promise( - (downloadComplete, downloadFailed) => { - fetch(this.binaryUrl) - .then(resp => { - if (resp.status !== 200 || resp.body === null) { - console.log('Download of emulator failed: ', resp.statusText); - downloadFailed(); - } else { - const reader = resp.body.getReader(); - reader.read().then(function readChunk({ done, value }): any { - if (done) { - console.log('done download. buffer length:', buf.length); - downloadComplete(); - } else { - buf.push(...value); - return reader.read().then(readChunk); - } - }); + // We want access to `this.binaryPath` after the download is finished, but we can't since in + // `readChunk`, `this` is not inherited from the parent (since it's a named function expression). + // To work around this, we wrap the fetch in a promise, then once it's resolved we can access `this` in the callback arrow function. + const downloadPromise = new Promise( + (downloadComplete, downloadFailed) => { + fetch(this.binaryUrl) + .then(resp => { + if (!resp.ok || resp.body === null) { + return downloadFailed( + `Failed to download emulator: [${resp.status}] ${resp.statusText}` + ); + } + + const buf = new Uint8Array(2 ** 25); // 32Mb + let cur = 0; + const reader = resp.body.getReader(); + reader.read().then(function readChunk({ done, value }): any { + if (done) { + return downloadComplete(buf); } - }) - .catch(e => { - console.log(`Download of emulator failed: ${e}`); - downloadFailed(); + + if (!value) { + return downloadFailed( + 'Did not receive chunk in response body' + ); + } + + buf.set(value, cur); + cur += value.length; + return reader.read().then(readChunk); }); - } - ); + }) + .catch(err => downloadFailed(err)); + } + ); - downloadPromise.then( - () => { - console.log('Download complete'); - // Change emulator binary file permission to 'rwxr-xr-x'. - // The execute permission is required for it to be able to start - // with 'java -jar'. - fs.writeFileSync(filepath, new Uint8Array(buf)); - - fs.chmod(filepath, 0o755, err => { - if (err) { - reject(err); - } + downloadPromise.then(buf => { + fs.writeFileSync(filepath, buf); + fs.chmod(filepath, 0o755, err => { + if (err) { + return reject(err); + } - console.log(`Changed emulator file permissions to 'rwxr-xr-x'.`); - this.binaryPath = filepath; - if (this.copyToCache()) { - console.log(`Cached emulator at ${this.cacheBinaryPath}`); - } - resolve(); - }); - }, - () => { - reject(); + console.log(`Changed emulator file permissions to 'rwxr-xr-x'.`); + // Since we are now in an arrow function, `this` is inherited from the `download()` method, so it is the Emulator object + this.binaryPath = filepath; + if (this.copyToCache()) { + console.log(`Cached emulator at ${this.cacheBinaryPath}`); } - ); + + resolve(); + }); }); }); } @@ -128,14 +119,11 @@ export abstract class Emulator { } let promise: ChildProcessPromise; if (this.isDataConnect) { - promise = spawn( - this.binaryPath, - [ - 'dev', - '--local_connection_string', - "'postgresql://postgres:secretpassword@localhost:5432/postgres?sslmode=disable'" - ] - ); + promise = spawn(this.binaryPath, [ + 'dev', + '--local_connection_string', + "'postgresql://postgres:secretpassword@localhost:5432/postgres?sslmode=disable'" + ]); } else { promise = spawn( 'java', From cf6ce4e82e759433970893e667c7bbe9839452bd Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Tue, 15 Oct 2024 10:31:49 -0400 Subject: [PATCH 4/7] cleanup --- .github/workflows/test-all.yml | 5 ++--- scripts/emulator-testing/emulators/emulator.ts | 13 ++++++++++--- 2 files changed, 12 insertions(+), 6 deletions(-) diff --git a/.github/workflows/test-all.yml b/.github/workflows/test-all.yml index 3928612f6b..eda081d1df 100644 --- a/.github/workflows/test-all.yml +++ b/.github/workflows/test-all.yml @@ -14,7 +14,7 @@ name: Test All Packages -on: push +on: pull_request env: # make chromedriver detect installed Chrome version and download the corresponding driver @@ -92,8 +92,7 @@ jobs: - name: Run unit tests # Ignore auth and firestore since they're handled in their own separate jobs. run: | - # xvfb-run yarn lerna run --ignore '{firebase-messaging-integration-test,@firebase/auth*,@firebase/firestore*,firebase-firestore-integration-test}' test:ci - xvfb-run yarn lerna run --scope @firebase/data-connect test + xvfb-run yarn lerna run --ignore '{firebase-messaging-integration-test,@firebase/auth*,@firebase/firestore*,firebase-firestore-integration-test}' test:ci node scripts/print_test_logs.js env: FIREBASE_TOKEN: ${{ secrets.FIREBASE_CLI_TOKEN }} diff --git a/scripts/emulator-testing/emulators/emulator.ts b/scripts/emulator-testing/emulators/emulator.ts index 4688ab615e..988128250a 100644 --- a/scripts/emulator-testing/emulators/emulator.ts +++ b/scripts/emulator-testing/emulators/emulator.ts @@ -56,9 +56,16 @@ export abstract class Emulator { const { name: tempDir } = tmp.dirSync({ unsafeCleanup: true }); const filepath = path.resolve(tempDir, this.binaryName); return new Promise((resolve, reject) => { - // We want access to `this.binaryPath` after the download is finished, but we can't since in - // `readChunk`, `this` is not inherited from the parent (since it's a named function expression). - // To work around this, we wrap the fetch in a promise, then once it's resolved we can access `this` in the callback arrow function. + /** + * Once the download is `done` in `readChunk`, we want to set `this.binaryPath` to the path of the + * downloaded emulator. Unfortunately, we can't access `this` when inside `readChunk`'s scope, since + * it's a named function expression, and does not inherit the `this` object from it's parent. + * To work around this, we wrap the fetch in a promise, + * then once it's resolved we can access `this` in a callback arrow function that *does* inherit + * `this` from the parent object, allowing us to set `this.binaryPath`. + * Note that we can't make readChunk an arrow function, since it needs to be named so that we can + * perform recursion to read the next chunk. + */ const downloadPromise = new Promise( (downloadComplete, downloadFailed) => { fetch(this.binaryUrl) From 840534a5099e3c6283297665644a770e4069d29d Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Tue, 15 Oct 2024 10:42:38 -0400 Subject: [PATCH 5/7] Format --- scripts/emulator-testing/emulators/emulator.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scripts/emulator-testing/emulators/emulator.ts b/scripts/emulator-testing/emulators/emulator.ts index 988128250a..b7987f7c83 100644 --- a/scripts/emulator-testing/emulators/emulator.ts +++ b/scripts/emulator-testing/emulators/emulator.ts @@ -57,10 +57,10 @@ export abstract class Emulator { const filepath = path.resolve(tempDir, this.binaryName); return new Promise((resolve, reject) => { /** - * Once the download is `done` in `readChunk`, we want to set `this.binaryPath` to the path of the + * Once the download is `done` in `readChunk`, we want to set `this.binaryPath` to the path of the * downloaded emulator. Unfortunately, we can't access `this` when inside `readChunk`'s scope, since * it's a named function expression, and does not inherit the `this` object from it's parent. - * To work around this, we wrap the fetch in a promise, + * To work around this, we wrap the fetch in a promise, * then once it's resolved we can access `this` in a callback arrow function that *does* inherit * `this` from the parent object, allowing us to set `this.binaryPath`. * Note that we can't make readChunk an arrow function, since it needs to be named so that we can From 01eebb7cd12ed0fa4d752d9cd207b30686aa166e Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Tue, 15 Oct 2024 11:19:09 -0400 Subject: [PATCH 6/7] increase buffer size and slice before write --- .../emulator-testing/emulators/emulator.ts | 42 ++++++++++--------- 1 file changed, 23 insertions(+), 19 deletions(-) diff --git a/scripts/emulator-testing/emulators/emulator.ts b/scripts/emulator-testing/emulators/emulator.ts index b7987f7c83..745db59623 100644 --- a/scripts/emulator-testing/emulators/emulator.ts +++ b/scripts/emulator-testing/emulators/emulator.ts @@ -53,8 +53,11 @@ export abstract class Emulator { return Promise.resolve(); } + console.log(`Downloading emulator from [${this.binaryUrl}] ...`); const { name: tempDir } = tmp.dirSync({ unsafeCleanup: true }); const filepath = path.resolve(tempDir, this.binaryName); + let cur = 0; + let buf = new Uint8Array(2 ** 26); return new Promise((resolve, reject) => { /** * Once the download is `done` in `readChunk`, we want to set `this.binaryPath` to the path of the @@ -66,7 +69,7 @@ export abstract class Emulator { * Note that we can't make readChunk an arrow function, since it needs to be named so that we can * perform recursion to read the next chunk. */ - const downloadPromise = new Promise( + const downloadPromise = new Promise( (downloadComplete, downloadFailed) => { fetch(this.binaryUrl) .then(resp => { @@ -76,12 +79,10 @@ export abstract class Emulator { ); } - const buf = new Uint8Array(2 ** 25); // 32Mb - let cur = 0; const reader = resp.body.getReader(); reader.read().then(function readChunk({ done, value }): any { if (done) { - return downloadComplete(buf); + return downloadComplete(); } if (!value) { @@ -99,23 +100,26 @@ export abstract class Emulator { } ); - downloadPromise.then(buf => { - fs.writeFileSync(filepath, buf); - fs.chmod(filepath, 0o755, err => { - if (err) { - return reject(err); - } + downloadPromise + .then(() => { + console.log('Download complete.'); + fs.writeFileSync(filepath, buf.slice(0, cur)); + fs.chmod(filepath, 0o755, err => { + if (err) { + return reject(err); + } - console.log(`Changed emulator file permissions to 'rwxr-xr-x'.`); - // Since we are now in an arrow function, `this` is inherited from the `download()` method, so it is the Emulator object - this.binaryPath = filepath; - if (this.copyToCache()) { - console.log(`Cached emulator at ${this.cacheBinaryPath}`); - } + console.log(`Changed emulator file permissions to 'rwxr-xr-x'.`); + // Since we are now in an arrow function, `this` is inherited from the `download()` method, so it is the Emulator object + this.binaryPath = filepath; + if (this.copyToCache()) { + console.log(`Cached emulator at ${this.cacheBinaryPath}`); + } - resolve(); - }); - }); + resolve(); + }); + }) + .catch(err => reject(err)); }); } From 8e44e2c2cc9089e00e0b9a01419fc21a157306ef Mon Sep 17 00:00:00 2001 From: Daniel La Rocque Date: Tue, 15 Oct 2024 11:38:05 -0400 Subject: [PATCH 7/7] Add comment to explain buffer size --- scripts/emulator-testing/emulators/emulator.ts | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/scripts/emulator-testing/emulators/emulator.ts b/scripts/emulator-testing/emulators/emulator.ts index 745db59623..a58324c50d 100644 --- a/scripts/emulator-testing/emulators/emulator.ts +++ b/scripts/emulator-testing/emulators/emulator.ts @@ -57,7 +57,12 @@ export abstract class Emulator { const { name: tempDir } = tmp.dirSync({ unsafeCleanup: true }); const filepath = path.resolve(tempDir, this.binaryName); let cur = 0; - let buf = new Uint8Array(2 ** 26); + // The emulators vary in size and are approximately 32MiB. If we define this array to be + // only 32MiB, there's a risk in the future that the download will suddenly fail because there + // was an increase in emulator size, and the buffer is no longer large enough. + // To prevent this, we give some room for an increase in size of the emulators + // and make the buffer 64MiB. + let buf = new Uint8Array(2 ** 26); // 64 MiB return new Promise((resolve, reject) => { /** * Once the download is `done` in `readChunk`, we want to set `this.binaryPath` to the path of the