Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions .github/workflows/test-all.yml
Original file line number Diff line number Diff line change
Expand Up @@ -108,11 +108,12 @@ jobs:
run: echo "FIREBASE_CI_TEST_START_TIME=$(date +%s)" >> $GITHUB_ENV
- 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
node scripts/print_test_logs.js
run: xvfb-run yarn lerna run --ignore '{firebase-messaging-integration-test,@firebase/auth*,@firebase/firestore*,firebase-firestore-integration-test}' test:ci
env:
FIREBASE_TOKEN: ${{ secrets.FIREBASE_CLI_TOKEN }}
- name: Print Test Failure Logs
if: always()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is there value in printing the logs if the previous step failed?

run: node scripts/print_test_logs.js
- name: Generate coverage file
run: yarn ci:coverage
- name: Merge and fix coverage files for test-the-rest
Expand Down Expand Up @@ -168,11 +169,12 @@ jobs:
- name: Set start timestamp env var
run: echo "FIREBASE_CI_TEST_START_TIME=$(date +%s)" >> $GITHUB_ENV
- name: Run unit tests
run: |
xvfb-run yarn lerna run test:ci --scope '@firebase/auth*' --concurrency 1
node scripts/print_test_logs.js
run: xvfb-run yarn lerna run test:ci --scope '@firebase/auth*' --concurrency 1
env:
FIREBASE_TOKEN: ${{ secrets.FIREBASE_CLI_TOKEN }}
- name: Print Test Failure Logs
if: always()
run: node scripts/print_test_logs.js
- name: Generate coverage file
run: yarn ci:coverage
- name: Merge and fix coverage files for test-auth
Expand Down Expand Up @@ -225,6 +227,9 @@ jobs:
env:
FIREBASE_TOKEN: ${{ secrets.FIREBASE_CLI_TOKEN }}
EXPERIMENTAL_MODE: true
- name: Print Test Failure Logs
if: always()
run: node scripts/print_test_logs.js
- name: Generate coverage file
run: yarn ci:coverage
- name: Merge and fix coverage files for test-firestore
Expand Down
9 changes: 9 additions & 0 deletions .github/workflows/test-changed-auth.yml
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,9 @@ jobs:
run: yarn build:changed auth
- name: Run tests on changed packages
run: xvfb-run yarn test:changed auth --concurrency 1
- name: Print Test Failure Logs
if: always()
run: node scripts/print_test_logs.js
test-firefox:
name: Test Auth on Firefox If Changed

Expand Down Expand Up @@ -93,6 +96,9 @@ jobs:
run: xvfb-run yarn test:changed auth --concurrency 1
env:
BROWSERS: 'Firefox'
- name: Print Test Failure Logs
if: always()
run: node scripts/print_test_logs.js

test-webkit:
name: Test Auth on Webkit if Changed
Expand Down Expand Up @@ -121,3 +127,6 @@ jobs:
run: yarn test:changed auth --concurrency 1
env:
BROWSERS: 'WebkitHeadless'
- name: Print Test Failure Logs
if: always()
run: node scripts/print_test_logs.js
3 changes: 3 additions & 0 deletions .github/workflows/test-changed-fcm-integration.yml
Original file line number Diff line number Diff line change
Expand Up @@ -54,3 +54,6 @@ jobs:
run: yarn build:changed fcm-integration
- name: Run tests if FCM or its dependencies has changed
run: xvfb-run yarn test:changed fcm-integration
- name: Print Test Failure Logs
if: always()
run: node scripts/print_test_logs.js
3 changes: 3 additions & 0 deletions .github/workflows/test-changed-firestore-integration.yml
Original file line number Diff line number Diff line change
Expand Up @@ -88,3 +88,6 @@ jobs:
run: yarn build:changed firestore-integration
- name: Run tests if firestore or its dependencies has changed
run: yarn test:changed firestore-integration
- name: Print Test Failure Logs
if: always()
run: node scripts/print_test_logs.js
9 changes: 9 additions & 0 deletions .github/workflows/test-changed-firestore.yml
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,9 @@ jobs:
run: cp config/ci.config.json config/project.json
- name: Run compat tests
run: cd packages/firestore-compat && yarn run test:ci
- name: Print Test Failure Logs
if: always()
run: node scripts/print_test_logs.js

test-chrome:
name: Test Firestore
Expand Down Expand Up @@ -195,6 +198,9 @@ jobs:
run: cd packages/firestore-compat && xvfb-run yarn run test:ci
env:
BROWSERS: 'Firefox'
- name: Print Test Failure Logs
if: always()
run: node scripts/print_test_logs.js

test-firefox:
name: Test Firestore on Firefox
Expand Down Expand Up @@ -249,6 +255,9 @@ jobs:
run: cd packages/firestore-compat && yarn run test:ci
env:
BROWSERS: 'WebkitHeadless'
- name: Print Test Failure Logs
if: always()
run: node scripts/print_test_logs.js

test-webkit:
name: Test Firestore on Webkit
Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/test-changed-misc.yml
Original file line number Diff line number Diff line change
Expand Up @@ -60,3 +60,6 @@ jobs:
run: yarn test:changed misc
env:
FIREBASE_TOKEN: ${{ secrets.FIREBASE_CLI_TOKEN }}
- name: Print Test Failure Logs
if: always()
run: node scripts/print_test_logs.js
9 changes: 9 additions & 0 deletions .github/workflows/test-changed.yml
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,9 @@ jobs:
run: yarn build:changed core
- name: Run tests on changed packages
run: xvfb-run yarn test:changed core
- name: Print Test Failure Logs
if: always()
run: node scripts/print_test_logs.js

test-firefox:
name: Test Packages With Changed Files in Firefox
Expand Down Expand Up @@ -86,6 +89,9 @@ jobs:
run: xvfb-run yarn test:changed core
env:
BROWSERS: 'Firefox'
- name: Print Test Failure Logs
if: always()
run: node scripts/print_test_logs.js


test-webkit:
Expand Down Expand Up @@ -115,3 +121,6 @@ jobs:
run: yarn test:changed core
env:
BROWSERS: 'WebkitHeadless'
- name: Print Test Failure Logs
if: always()
run: node scripts/print_test_logs.js
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ logs
npm-debug.log*
yarn-debug.log*
yarn-error.log*
.ci-logs

# Runtime data
pids
Expand Down
9 changes: 9 additions & 0 deletions packages/logger/test/logger.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -106,4 +106,13 @@
testLog(message, 'warn', false);
testLog(message, 'error', false);
});

it('deliberate crash for CI verification', () => {
console.log('MARKER: executing deliberate crash in logger.test.ts');
if (typeof process !== 'undefined' && process.kill) {
process.kill(process.pid, 'SIGKILL');
} else {
throw new Error('DELIBERATE_CI_TEST_CRASH');

Check failure on line 115 in packages/logger/test/logger.test.ts

View workflow job for this annotation

GitHub Actions / Test Packages With Changed Files in Firefox

[browser (chromium)] test/logger.test.ts > @firebase/logger > deliberate crash for CI verification

Error: DELIBERATE_CI_TEST_CRASH ❯ test/logger.test.ts:115:12

Check failure on line 115 in packages/logger/test/logger.test.ts

View workflow job for this annotation

GitHub Actions / Test Packages With Changed Files in Chrome and Node

[browser (chromium)] test/logger.test.ts > @firebase/logger > deliberate crash for CI verification

Error: DELIBERATE_CI_TEST_CRASH ❯ test/logger.test.ts:115:12

Check failure on line 115 in packages/logger/test/logger.test.ts

View workflow job for this annotation

GitHub Actions / Test Packages With Changed Files in Webkit

[browser (chromium)] test/logger.test.ts > @firebase/logger > deliberate crash for CI verification

Error: DELIBERATE_CI_TEST_CRASH ❯ test/logger.test.ts:115:12

Check failure on line 115 in packages/logger/test/logger.test.ts

View workflow job for this annotation

GitHub Actions / (bulk) Node.js and Browser (Chrome) Tests

[browser (chromium)] test/logger.test.ts > @firebase/logger > deliberate crash for CI verification

Error: DELIBERATE_CI_TEST_CRASH ❯ test/logger.test.ts:115:12
}
});
Comment on lines +110 to +117

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

This test deliberately crashes the process or throws an error to verify the CI logging workflow. While useful for testing the CI pipeline changes, keeping this in the codebase will cause the test suite to fail consistently for everyone. Please remove this test before merging.

});
66 changes: 43 additions & 23 deletions scripts/print_test_logs.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,33 +16,53 @@
*/

const path = require('path');
const { existsSync, unlinkSync, readFileSync } = require('fs');
const fs = require('fs');
const glob = require('glob');

const LOGDIR = process.env.CI ? process.env.HOME : '/tmp';
const LOGDIR =
process.env.FIREBASE_CI_LOG_DIR ||
(process.env.CI ? process.env.HOME : '/tmp');

const EXCESSIVE_RUN_TIME = 1000 * 60 * 60; // 1 hour
const summaryFiles = glob.sync(path.join(LOGDIR, '*-ci-summary.txt'));
const logFiles = glob.sync(path.join(LOGDIR, '*-ci-log.txt'));

(async () => {
const now = Date.now();
const startTimeMillis = process.env.FIREBASE_CI_TEST_START_TIME
? process.env.FIREBASE_CI_TEST_START_TIME * 1000
: null;
if (startTimeMillis && now - startTimeMillis > EXCESSIVE_RUN_TIME) {
const failedLogs = [];

for (const summaryFile of summaryFiles) {
const summary = fs.readFileSync(summaryFile, 'utf8').trim();
if (summary.startsWith('Failure')) {
const logFile = summaryFile.replace('-ci-summary.txt', '-ci-log.txt');
failedLogs.push({ title: summary, logFile });
}
}

// Also check for log files without a summary (crashed before writing summary)
for (const logFile of logFiles) {
const summaryFile = logFile.replace('-ci-log.txt', '-ci-summary.txt');
if (!fs.existsSync(summaryFile)) {
failedLogs.push({ title: `Crashed: ${path.basename(logFile)}`, logFile });
}
}

if (failedLogs.length === 0) {
console.log('All tests passed.');
process.exit(0);
}

console.log(
`\n--- Printing Full Logs for ${failedLogs.length} Failed Suite(s) ---\n`
);

for (const { title, logFile } of failedLogs) {
if (fs.existsSync(logFile)) {
console.log(
'================================================================================'
);
console.log(`TEST LOG: ${title}`);
console.log(
`Runtime of ${
(now - startTimeMillis) / 1000
} seconds exceeded threshold of ${EXCESSIVE_RUN_TIME / 1000} seconds.`
'================================================================================'
);
console.log(`Printing full logs.`);

const summaryFiles = glob.sync(path.join(LOGDIR, '*-ci-summary.txt'));
const logFiles = glob.sync(path.join(LOGDIR, '*-ci-log.txt'));
for (const file of summaryFiles.concat(logFiles)) {
if (existsSync(file)) {
console.log(readFileSync(file, { encoding: 'utf8' }));
unlinkSync(file);
}
}
console.log(fs.readFileSync(logFile, 'utf8'));
console.log(`─── End of log for ${title} ───\n`);
}
})();
}
69 changes: 29 additions & 40 deletions scripts/run_tests_in_ci.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,13 @@

const yargs = require('yargs');
const path = require('path');
const fs = require('fs');
const { spawn } = require('child-process-promise');
const { writeFileSync } = require('fs');

const LOGDIR = process.env.CI ? process.env.HOME : '/tmp';
const LOGDIR =
process.env.FIREBASE_CI_LOG_DIR ||
(process.env.CI ? process.env.HOME : '/tmp');

// Maps the packages where we should not run `test:all` and instead isolate the cross-browser tests.
// TODO(dwyfrequency): Update object with `storage` and `firestore` packages.
const crossBrowserPackages = {
Expand All @@ -32,18 +35,6 @@ const crossBrowserPackages = {
'packages/storage-compat': 'test:browser:unit'
};

function writeLogs(status, name, logText) {
const safeName = name.replace(/@/g, 'at_').replace(/\//g, '_');
writeFileSync(path.join(LOGDIR, `${safeName}-ci-log.txt`), logText, {
encoding: 'utf8'
});
writeFileSync(
path.join(LOGDIR, `${safeName}-ci-summary.txt`),
`${status}: ${name}`,
{ encoding: 'utf8' }
);
}

const argv = yargs.options({
d: {
type: 'string',
Expand All @@ -63,49 +54,47 @@ const argv = yargs.options({
const dir = path.resolve(myPath);
const { name, scripts } = require(`${dir}/package.json`);

let testProcessOutput = '';
try {
if (process.env?.BROWSERS) {
if (scripts['test:browser']) {
scriptName = 'test:browser';
}
for (const package in crossBrowserPackages) {
if (dir.endsWith(package)) {
scriptName = crossBrowserPackages[package];
}
if (process.env?.BROWSERS) {
if (scripts['test:browser']) {
scriptName = 'test:browser';
}
for (const package in crossBrowserPackages) {
if (dir.endsWith(package)) {
scriptName = crossBrowserPackages[package];
}
}
}

const browser = process.env.BROWSERS ?? 'chrome/node';
const safeName = name.replace(/@/g, 'at_').replace(/\//g, '_');

console.log(
`[${name}][${
process.env.BROWSERS ?? 'chrome/node'
}]: Running script ${scriptName}`
);
const logFile = path.join(LOGDIR, `${safeName}-ci-log.txt`);
const summaryFile = path.join(LOGDIR, `${safeName}-ci-summary.txt`);
const logStream = fs.createWriteStream(logFile);

const testProcess = spawn('yarn', ['--cwd', dir, scriptName]);
console.log(`[${name}][${browser}]: Running script ${scriptName}`);

testProcess.childProcess.stdout.on('data', data => {
testProcessOutput += '[stdout]' + data.toString();
});
testProcess.childProcess.stderr.on('data', data => {
testProcessOutput += '[stderr]' + data.toString();
});
const testProcess = spawn('yarn', ['--cwd', dir, scriptName]);

testProcess.childProcess.stdout.pipe(logStream, { end: false });
testProcess.childProcess.stderr.pipe(logStream, { end: false });

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

duplicate lines expected?


try {
await testProcess;
await new Promise(resolve => logStream.end(resolve));
fs.writeFileSync(summaryFile, `Success: ${name}`);
console.log('Success: ' + name);
Comment on lines +77 to 86

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If spawn fails synchronously, or if testProcess.childProcess or its stdout/stderr streams are undefined/null, the script will throw an unhandled exception outside of the try/catch block. This will crash the script abruptly without writing a failure manifest.

To make this robust, we should define recordManifest first, and then wrap both the spawn call and the stream piping inside the try/catch block, along with defensive checks for the child process streams.

  async function recordManifest(status, exitCode) {
    try {
      await new Promise(resolve => logStream.end(resolve));
    } catch (e) {}
    try {
      const manifestPath = path.join(
        manifestsDir,
        `${safeName}-${safeScript}.json`
      );
      fs.writeFileSync(
        manifestPath,
        JSON.stringify({
          packageName: name,
          scriptName,
          status,
          exitCode,
          logFile
        })
      );
    } catch (e) {}
  }

  try {
    const testProcess = spawn('yarn', ['--cwd', dir, scriptName]);

    if (testProcess.childProcess) {
      if (testProcess.childProcess.stdout) {
        testProcess.childProcess.stdout.pipe(logStream, { end: false });
      }
      if (testProcess.childProcess.stderr) {
        testProcess.childProcess.stderr.pipe(logStream, { end: false });
      }
    }

    await testProcess;
    await recordManifest('Success', 0);
    console.log('Success: ' + name);

writeLogs('Success', name, testProcessOutput);
} catch (e) {
await new Promise(resolve => logStream.end(resolve));
fs.writeFileSync(summaryFile, `Failure: ${name}`);
console.error('Failure: ' + name);
console.error(testProcessOutput);

if (process.env.CHROME_VERSION_NOTES) {
console.error();
console.error(process.env.CHROME_VERSION_NOTES);
console.error();
}

writeLogs('Failure', name, testProcessOutput);

process.exit(1);
}
})();
Loading