Skip to content

Fix Saucelabs tests for Firestore unit tests except IE #3095

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
May 29, 2020
Merged
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
11 changes: 10 additions & 1 deletion config/karma.saucelabs.js
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,14 @@ const packageConfigs = {
messaging: {
// Messaging currently only supports these browsers.
browsers: ['Chrome_Windows', 'Firefox_Windows', 'Edge_Windows']
},
firestore: {
browsers: [
'Chrome_Windows',
'Firefox_Windows',
'Edge_Windows',
'Safari_macOS'
]
}
};

Expand Down Expand Up @@ -170,7 +178,8 @@ module.exports = function(config) {
'packages/polyfill/index.ts': ['webpack', 'sourcemap'],
'**/test/**/*.ts': ['webpack', 'sourcemap'],
'**/*.test.ts': ['webpack', 'sourcemap'],
'packages/firestore/test/**/bootstrap.ts': ['webpack', 'babel'],
// Restore when ready to run Firestore unit tests in IE.
// 'packages/firestore/test/**/bootstrap.ts': ['webpack', 'babel'],
'integration/**/namespace.*': ['webpack', 'babel', 'sourcemap']
},

Expand Down
1 change: 0 additions & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@
"test:changed": "node scripts/run_changed.js",
"test:setup": "node tools/config.js",
"test:saucelabs": "node scripts/run_saucelabs.js",
"test:saucelabs:single": "karma start config/karma.saucelabs.js --single-run",
"docgen:js": "node scripts/docgen/generate-docs.js --api js",
"docgen:node": "node scripts/docgen/generate-docs.js --api node",
"docgen": "yarn docgen:js; yarn docgen:node",
Expand Down
4 changes: 1 addition & 3 deletions packages/firestore/karma.conf.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2017 Google Inc.
* Copyright 2017 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -15,8 +15,6 @@
* limitations under the License.
*/

const karma = require('karma');
const path = require('path');
const karmaBase = require('../../config/karma.base');
const { argv } = require('yargs');

Expand Down
24 changes: 19 additions & 5 deletions packages/firestore/src/util/async_queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ export class AsyncQueue {
.catch((error: FirestoreError) => {
this.failure = error;
this.operationInProgress = false;
const message = error.stack || error.message || '';
const message = getMessageOrStack(error);
logError('INTERNAL UNHANDLED ERROR: ', message);

// Re-throw the error so that this.tail becomes a rejected Promise and
Expand Down Expand Up @@ -413,10 +413,7 @@ export class AsyncQueue {

private verifyNotFailed(): void {
if (this.failure) {
fail(
'AsyncQueue is already failed: ' +
(this.failure.stack || this.failure.message)
);
fail('AsyncQueue is already failed: ' + getMessageOrStack(this.failure));
}
}

Expand Down Expand Up @@ -518,3 +515,20 @@ export function wrapInUserErrorIfRecoverable(
throw e;
}
}

/**
* Chrome includes Error.message in Error.stack. Other browsers do not.
* This returns expected output of message + stack when available.
* @param error Error or FirestoreError
*/
function getMessageOrStack(error: Error): string {
let message = error.message || '';
if (error.stack) {
if (error.stack.includes(error.message)) {
message = error.stack;
} else {
message = error.message + '\n' + error.stack;
}
}
return message;
}
5 changes: 3 additions & 2 deletions packages/firestore/test/unit/core/event_manager.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -224,9 +224,10 @@ describe('QueryListener', () => {
const query = Query.atPath(path('rooms/Eros'));

const listener = queryListener(query, [], events);
const error = new Error('bad');

listener.onError(Error('bad'));
expect(events[0]).to.deep.equal(new Error('bad'));
listener.onError(error);
expect(events[0]).to.deep.equal(error);
});

it('raises event for empty collection after sync', () => {
Expand Down
5 changes: 4 additions & 1 deletion packages/firestore/test/unit/util/async_queue.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import { getLogLevel, setLogLevel, LogLevel } from '../../../src/util/log';
import { Deferred, Rejecter, Resolver } from '../../../src/util/promise';
import { fail } from '../../../src/util/assert';
import { IndexedDbTransactionError } from '../../../src/local/simple_db';
import { isSafari } from '@firebase/util';

use(chaiAsPromised);

Expand Down Expand Up @@ -137,7 +138,9 @@ describe('AsyncQueue', () => {
});
});

it('can schedule ops in the future', async () => {
// Flaky on Safari.
Copy link
Member

Choose a reason for hiding this comment

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

Do we know why it is flaky on Safari?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I haven't been able to find any reason. The test relies on two setTimeouts at 1ms and 5ms happening in the correct order, and sometimes Safari doesn't seem to get the order right. I can't find any known bug or issue that might cause that.

// eslint-disable-next-line no-restricted-properties
(isSafari() ? it.skip : it)('can schedule ops in the future', async () => {
const queue = new AsyncQueue();
const completedSteps: number[] = [];
const doStep = (n: number): Promise<number> =>
Expand Down
9 changes: 0 additions & 9 deletions packages/firestore/test/util/test_platform.ts
Original file line number Diff line number Diff line change
Expand Up @@ -271,12 +271,3 @@ export class TestPlatform implements Platform {
return this.basePlatform.randomBytes(nBytes);
}
}

/** Returns true if we are running under Node. */
export function isNode(): boolean {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Seems like this wasn't being used anywhere, and there's a better version in @firebase/util so I hope it's okay to remove?

Copy link
Contributor

Choose a reason for hiding this comment

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

@wu-hui might have just started using this. He would probably be delighted to switch to the better version :)

return (
typeof process !== 'undefined' &&
process.title !== undefined &&
process.title.indexOf('node') !== -1
);
}
9 changes: 9 additions & 0 deletions packages/util/src/environment.ts
Original file line number Diff line number Diff line change
Expand Up @@ -125,3 +125,12 @@ export function isUWP(): boolean {
export function isNodeSdk(): boolean {
return CONSTANTS.NODE_CLIENT === true || CONSTANTS.NODE_ADMIN === true;
}

/** Returns true if we are running in Safari. */
export function isSafari(): boolean {
return (
!isNode() &&
navigator.userAgent.includes('Safari') &&
!navigator.userAgent.includes('Chrome')
);
}
28 changes: 13 additions & 15 deletions scripts/run_saucelabs.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
/**
* @license
* Copyright 2019 Google Inc.
* Copyright 2019 Google LLC
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -44,8 +44,7 @@ const testFiles = configFiles.length
// Get CI build number or generate one if running locally.
const buildNumber =
process.env.TRAVIS_BUILD_NUMBER ||
// GitHub Actions does not have a build number, but the feature has been requested.
process.env.GITHUB_SHA ||
process.env.GITHUB_RUN_ID ||
`local_${process.env.USER}_${new Date().getTime()}`;

/**
Expand All @@ -67,26 +66,25 @@ async function runTest(testFile) {
});
}
}
return runKarma(testFile);
}

const promise = spawn('yarn', [
'test:saucelabs:single',
async function runKarma(testFile) {
const karmaArgs = [
'karma',
'start',
'config/karma.saucelabs.js',
'--single-run',
'--testConfigFile',
testFile,
'--buildNumber',
buildNumber
]);
];

const promise = spawn('npx', karmaArgs, { stdio: 'inherit' });
const childProcess = promise.childProcess;
let exitCode = 0;

childProcess.stdout.on('data', data => {
console.log(`[${testFile}]:`, data.toString());
});

// Lerna's normal output goes to stderr for some reason.
childProcess.stderr.on('data', data => {
console.log(`[${testFile}]:`, data.toString());
});

// Capture exit code of this single package test run
childProcess.on('exit', code => {
exitCode = code;
Expand Down