Skip to content

Add Firefox testing to Auth #6522

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.

Sign up for GitHub

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 14 commits into from
Aug 15, 2022
Merged

Add Firefox testing to Auth #6522

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
312a8df
Update version test to accomidate multiple browsers
dwyfrequency Aug 5, 2022
02b8594
Add notes and console.logs
dwyfrequency Aug 8, 2022
ed1eab6
Update tests to accomidate firefox browser
dwyfrequency Aug 9, 2022
9a41a60
Remove console.log
dwyfrequency Aug 9, 2022
efbd1b9
Update browsers based on environment variable
dwyfrequency Aug 9, 2022
cc8f7be
Update browsers when unit test selected
dwyfrequency Aug 10, 2022
45b6da7
Update karma config to take env variable
dwyfrequency Aug 10, 2022
68ab1b5
Add test github action
dwyfrequency Aug 11, 2022
623d0cc
Update github workflow
dwyfrequency Aug 11, 2022
66fa034
Update build command
dwyfrequency Aug 11, 2022
e8e4ed2
Update format in test changed auth
dwyfrequency Aug 11, 2022
9967d97
Remove stand alone action for firefox auth
dwyfrequency Aug 12, 2022
b5925b0
Add expect for spys, and test command for browser unit
dwyfrequency Aug 12, 2022
c52f331
Update spy to use proper sinon chai syntax
dwyfrequency Aug 15, 2022
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
86 changes: 60 additions & 26 deletions .github/workflows/test-changed-auth.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,32 +7,66 @@ env:
DETECT_CHROMEDRIVER_VERSION: true

jobs:
test:
name: Test Auth If Changed
test-chrome:
name: Test Auth on Chrome and Node If Changed
runs-on: ubuntu-latest

steps:
# install Chrome first, so the correct version of webdriver can be installed by chromedriver when setting up the repo
- name: install Chrome stable
run: |
sudo apt-get update
sudo apt-get install google-chrome-stable
- name: Checkout Repo
uses: actions/checkout@master
with:
# This makes Actions fetch all Git history so run-changed script can diff properly.
fetch-depth: 0
- name: Set up Node (14)
uses: actions/setup-node@v2
with:
node-version: 14.x
- name: Bump Node memory limit
run: echo "NODE_OPTIONS=--max_old_space_size=4096" >> $GITHUB_ENV
- name: Test setup and yarn install
run: |
cp config/ci.config.json config/project.json
yarn
- name: build
run: yarn build:changed auth
- name: Run tests on changed packages
run: xvfb-run yarn test:changed auth
# install Chrome first, so the correct version of webdriver can be installed by chromedriver when setting up the repo
- name: install Chrome stable
run: |
sudo apt-get update
sudo apt-get install google-chrome-stable
- name: Checkout Repo
uses: actions/checkout@master
with:
# This makes Actions fetch all Git history so run-changed script can diff properly.
fetch-depth: 0
- name: Set up Node (14)
uses: actions/setup-node@v2
with:
node-version: 14.x
- name: Bump Node memory limit
run: echo "NODE_OPTIONS=--max_old_space_size=4096" >> $GITHUB_ENV
- name: Test setup and yarn install
run: |
cp config/ci.config.json config/project.json
yarn
- name: build
run: yarn build:changed auth
- name: Run tests on changed packages
run: xvfb-run yarn test:changed auth
test-firefox:
name: Test Auth on Firefox If Changed
runs-on: ubuntu-latest

steps:
- name: install Firefox stable
run: |
sudo apt-get update
sudo apt-get install firefox
- name: Checkout Repo
uses: actions/checkout@master
with:
# This makes Actions fetch all Git history so run-changed script can diff properly.
fetch-depth: 0
- name: Set up Node (14)
uses: actions/setup-node@v2
with:
node-version: 14.x
- name: Bump Node memory limit
run: echo "NODE_OPTIONS=--max_old_space_size=4096" >> $GITHUB_ENV
- name: Test setup and yarn install
run: |
cp config/ci.config.json config/project.json
yarn
- name: build
run: yarn build:changed auth
- name: Run tests on auth changed packages
run: xvfb-run yarn --cwd packages/auth test:browser:unit
env:
BROWSERS: 'Firefox'
- name: Run tests on auth-compat changed packages
run: xvfb-run yarn --cwd packages/auth-compat test:browser:unit
env:
BROWSERS: 'Firefox'
9 changes: 9 additions & 0 deletions packages/auth-compat/karma.conf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ const files = ['src/**/*.test.ts'];

module.exports = function (config) {
const karmaConfig = Object.assign({}, karmaBase, {
browsers: getTestBrowsers(argv),
// files to load into karma
files: getTestFiles(),
preprocessors: { '**/*.ts': ['webpack', 'sourcemap'] },
Expand All @@ -43,6 +44,14 @@ function getTestFiles() {
}
}

function getTestBrowsers(argv) {
let browsers = ['ChromeHeadless'];
if (process.env?.BROWSERS && argv.unit) {
browsers = process.env?.BROWSERS?.split(',');
}
return browsers;
}

function getClientConfig() {
if (!argv.integration) {
return {};
Expand Down
1 change: 1 addition & 0 deletions packages/auth-compat/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@
"test:all": "run-p test:browser test:node test:integration",
"test:ci": "node ../../scripts/run_tests_in_ci.js -s test:all",
"test:browser": "karma start --single-run",
"test:browser:unit": "karma start --single-run --unit",
"test:browser:integration": "karma start --single-run --integration",
"test:node": "ts-node -O '{\"module\": \"commonjs\", \"target\": \"es6\"}' scripts/run_node_tests.ts",
"test:node:integration": "ts-node -O '{\"module\": \"commonjs\", \"target\": \"es6\"}' scripts/run_node_tests.ts --integration",
Expand Down
9 changes: 9 additions & 0 deletions packages/auth/karma.conf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ const { argv } = require('yargs');

module.exports = function (config) {
const karmaConfig = Object.assign({}, karmaBase, {
browsers: getTestBrowsers(argv),
// files to load into karma
files: getTestFiles(argv),
// frameworks to use
Expand Down Expand Up @@ -52,6 +53,14 @@ function getTestFiles(argv) {
}
}

function getTestBrowsers(argv) {
let browsers = ["ChromeHeadless"];
if (process.env?.BROWSERS && argv.unit) {
browsers = process.env?.BROWSERS?.split(',');
}
return browsers;
}

function getClientConfig(argv) {
if (!argv.local) {
return {};
Expand Down
7 changes: 4 additions & 3 deletions packages/auth/src/core/util/version.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,8 @@
import { SDK_VERSION } from '@firebase/app';
import { expect } from 'chai';
import { ClientPlatform, _getClientVersion } from './version';
import { isNode } from '@firebase/util';
import { getUA, isNode } from '@firebase/util';
import { _getBrowserName } from './browser';

describe('core/util/_getClientVersion', () => {
if (isNode()) {
Expand All @@ -33,15 +34,15 @@ describe('core/util/_getClientVersion', () => {
context('browser', () => {
it('should set the correct version', () => {
expect(_getClientVersion(ClientPlatform.BROWSER)).to.eq(
`Chrome/JsCore/${SDK_VERSION}/FirebaseCore-web`
`${_getBrowserName(getUA())}/JsCore/${SDK_VERSION}/FirebaseCore-web`
);
});
});

context('worker', () => {
it('should set the correct version', () => {
expect(_getClientVersion(ClientPlatform.WORKER)).to.eq(
`Chrome-Worker/JsCore/${SDK_VERSION}/FirebaseCore-web`
`${_getBrowserName(getUA())}-Worker/JsCore/${SDK_VERSION}/FirebaseCore-web`
);
});
});
Expand Down
6 changes: 4 additions & 2 deletions packages/auth/src/platform_browser/persistence/local_storage.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,15 @@ describe('platform_browser/persistence/local_storage', () => {
});

describe('#isAvailable', () => {
afterEach(() => sinon.restore());

it('should emit false if localStorage setItem throws', async () => {
sinon.stub(localStorage, 'setItem').throws(new Error('nope'));
sinon.stub(Storage.prototype, 'setItem').throws(new Error('nope'));
expect(await persistence._isAvailable()).to.be.false;
});

it('should emit false if localStorage removeItem throws', async () => {
sinon.stub(localStorage, 'removeItem').throws(new Error('nope'));
sinon.stub(Storage.prototype, 'removeItem').throws(new Error('nope'));
expect(await persistence._isAvailable()).to.be.false;
});

Expand Down
8 changes: 5 additions & 3 deletions packages/auth/src/platform_browser/persistence/session_storage.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,13 +63,15 @@ describe('platform_browser/persistence/session_storage', () => {
});

describe('#isAvailable', () => {
afterEach(() => sinon.restore());

it('should emit false if sessionStorage setItem throws', async () => {
sinon.stub(sessionStorage, 'setItem').throws(new Error('nope'));
expect(await persistence._isAvailable()).to.be.false;
sinon.stub(Storage.prototype, 'setItem').throws(new Error('nope'));
expect(await persistence._isAvailable()).to.be.false;
});

it('should emit false if sessionStorage removeItem throws', async () => {
sinon.stub(sessionStorage, 'removeItem').throws(new Error('nope'));
sinon.stub(Storage.prototype, 'removeItem').throws(new Error('nope'));
expect(await persistence._isAvailable()).to.be.false;
});

Expand Down
17 changes: 7 additions & 10 deletions packages/auth/src/platform_cordova/popup_redirect/events.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,8 @@ use(sinonChai);

describe('platform_cordova/popup_redirect/events', () => {
let auth: TestAuth;
let storageStub: sinon.SinonStubbedInstance<typeof localStorage>;

beforeEach(async () => {
auth = await testAuth();
storageStub = sinon.stub(localStorage);
});

afterEach(() => {
Expand Down Expand Up @@ -73,28 +70,28 @@ describe('platform_cordova/popup_redirect/events', () => {

describe('_savePartialEvent', () => {
it('sets the event', async () => {
const spy = sinon.spy(Storage.prototype, 'setItem');
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you explain why there is a change from stub to spy?

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 was having issues with the stub at one point and the test only required verifying if the method was called so stubbing out a return value was not necessary for the test itself.

const event = _generateNewEvent(auth, AuthEventType.REAUTH_VIA_REDIRECT);
await _savePartialEvent(auth, event);
expect(storageStub.setItem).to.have.been.calledWith(
'firebase:authEvent:test-api-key:test-app',
JSON.stringify(event)
);
expect(spy).to.have.been.calledWith('firebase:authEvent:test-api-key:test-app',
JSON.stringify(event));
Copy link
Contributor

Choose a reason for hiding this comment

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

What is this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

per https://blog.logrocket.com/storing-retrieving-javascript-objects-localstorage/, In the code block, we used the JSON.stringify() method to convert our JavaScript object into a string first because we can only store strings in the window.localStorage object. I didn't add the json stringify part. It was part of the original test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, sorry! I didn't see this was a line wrapping, weird indent.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually when I pasted it in and ran prettier inside VSCode, it indented it (the current version):

      expect(storageStub.calledWith('firebase:authEvent:test-api-key:test-app',
        JSON.stringify(event))).to.be.true;

Weird the formatter check didn't catch this? I ran yarn format and it didn't work but it does if I run prettier inside VSCode.

});
});

describe('_getAndRemoveEvent', () => {
it('returns null if no event is present', async () => {
storageStub.getItem.returns(null);
sinon.stub(Storage.prototype, 'getItem').returns(null);
expect(await _getAndRemoveEvent(auth)).to.be.null;
});

it('returns the event and deletes the key if present', async () => {
const event = JSON.stringify(
_generateNewEvent(auth, AuthEventType.REAUTH_VIA_REDIRECT)
);
storageStub.getItem.returns(event);
sinon.stub(Storage.prototype, 'getItem').returns(event);
const spy = sinon.spy(Storage.prototype, 'removeItem');
expect(await _getAndRemoveEvent(auth)).to.eql(JSON.parse(event));
expect(storageStub.removeItem).to.have.been.calledWith(
expect(spy).to.have.been.calledWith(
'firebase:authEvent:test-api-key:test-app'
);
});
Expand Down