Add Firefox testing to Auth #6522

dwyfrequency · 2022-08-09T18:59:25Z

No description provided.

changeset-bot · 2022-08-09T18:59:27Z

⚠️ No Changeset found

Latest commit: c52f331

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

github-actions · 2022-08-09T19:01:20Z

Changeset File Check ✅

No modified packages are missing from the changeset file.
No changeset formatting errors detected.

google-oss-bot · 2022-08-09T19:20:59Z

Size Report ¹

Affected Products

@firebase/database
Type Base (ac578e9) Merge (31c974b) Diff
browser 248 kB 248 kB -648 B (-0.3%)
esm5 276 kB 276 kB -574 B (-0.2%)
main 282 kB 281 kB -574 B (-0.2%)
module 248 kB 248 kB -648 B (-0.3%)
@firebase/database-compat/standalone
Type Base (ac578e9) Merge (31c974b) Diff
main 371 kB 370 kB -574 B (-0.2%)

`bundle`

Type	Base (ac578e9)	Merge (`31c974b`)	Diff
database (Listen for child events)	160 kB	160 kB	-230 B (-0.1%)
database (Listen for value events + Detach listeners)	160 kB	160 kB	-230 B (-0.1%)
database (Listen for value events)	160 kB	160 kB	-230 B (-0.1%)
database (Read data once)	157 kB	159 kB	+2.66 kB (+1.7%)
database (Save data as transactions)	162 kB	162 kB	-230 B (-0.1%)

`firebase`

17 size changes

Type	Base (ac578e9)	Merge (`31c974b`)	Diff
firebase-analytics.js	115 kB	24.9 kB	-89.6 kB (-78.2%)
firebase-app-check.js	90.9 kB	21.7 kB	-69.2 kB (-76.2%)
firebase-auth-cordova.js	472 kB	137 kB	-335 kB (-70.9%)
firebase-auth-react-native.js	498 kB	148 kB	-350 kB (-70.2%)
firebase-auth.js	418 kB	116 kB	-302 kB (-72.2%)
firebase-compat.js	795 kB	794 kB	-413 B (-0.1%)
firebase-database-compat.js	166 kB	166 kB	-413 B (-0.2%)
firebase-database.js	606 kB	153 kB	-453 kB (-74.7%)
firebase-firestore-lite.js	267 kB	86.1 kB	-181 kB (-67.8%)
firebase-firestore.js	852 kB	312 kB	-539 kB (-63.3%)
firebase-functions.js	32.0 kB	6.93 kB	-25.1 kB (-78.4%)
firebase-installations.js	59.6 kB	12.5 kB	-47.1 kB (-79.0%)
firebase-messaging-sw.js	107 kB	26.0 kB	-81.4 kB (-75.8%)
firebase-messaging.js	106 kB	24.8 kB	-81.2 kB (-76.6%)
firebase-performance.js	123 kB	30.7 kB	-92.6 kB (-75.1%)
firebase-remote-config.js	113 kB	26.4 kB	-86.4 kB (-76.6%)
firebase-storage.js	146 kB	36.7 kB	-109 kB (-74.9%)

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/4jy7PEmCTN.html

google-oss-bot · 2022-08-09T19:52:29Z

Size Analysis Report ¹

This report is too large (88,843 characters) to be displayed here in a GitHub comment. Please use the below link to see the full report on Google Cloud Storage.

Test Logs

https://storage.googleapis.com/firebase-sdk-metric-reports/FgABt5QWLO.html

hsubox76

Can you add a description? That's a good place to explain the plan overall plus any unusual changes (like spy vs stub?)

.github/workflows/test-changed-auth.yml

packages/auth/src/platform_browser/persistence/session_storage.test.ts

hsubox76 · 2022-08-12T17:28:15Z

packages/auth/src/platform_cordova/popup_redirect/events.test.ts

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

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


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

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.

hsubox76 · 2022-08-12T17:28:24Z

packages/auth/src/platform_cordova/popup_redirect/events.test.ts

-        'firebase:authEvent:test-api-key:test-app',
-        JSON.stringify(event)
-      );
+      spy.calledWith('firebase:authEvent:test-api-key:test-app',


Does this not need to be wrapped in expect()?

Yes, adding it now

hsubox76 · 2022-08-12T17:28:40Z

packages/auth/src/platform_cordova/popup_redirect/events.test.ts

-        JSON.stringify(event)
-      );
+      spy.calledWith('firebase:authEvent:test-api-key:test-app',
+      JSON.stringify(event));


What is this for?

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.

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

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.

sam-gc

We also have webdriver tests, but those are explicitly disabled in Firefox due to weird timing issues that we couldn't resolve. I don't imagine that's fixed now but just wanted to highlight

packages/auth/src/platform_cordova/popup_redirect/events.test.ts

sam-gc · 2022-08-15T15:09:30Z

packages/auth/src/platform_cordova/popup_redirect/events.test.ts

        'firebase:authEvent:test-api-key:test-app'
-      );
+      )).to.be.true;


sam-gc

Approved pending comments

dwyfrequency added 3 commits August 5, 2022 14:26

Update version test to accomidate multiple browsers

312a8df

Add notes and console.logs

02b8594

Update tests to accomidate firefox browser

ed1eab6

Remove console.log

9a41a60

dwyfrequency added 8 commits August 9, 2022 17:10

Update browsers based on environment variable

efbd1b9

Update browsers when unit test selected

cc8f7be

Update karma config to take env variable

45b6da7

Add test github action

68ab1b5

Update github workflow

623d0cc

Update build command

66fa034

Update format in test changed auth

e8e4ed2

Remove stand alone action for firefox auth

9967d97

hsubox76 reviewed Aug 12, 2022

View reviewed changes

Add expect for spys, and test command for browser unit

b5925b0

hsubox76 approved these changes Aug 12, 2022

View reviewed changes

dwyfrequency marked this pull request as ready for review August 12, 2022 20:22

dwyfrequency requested review from avolkovi, lisajian, sam-gc and yuchenshi as code owners August 12, 2022 20:22

sam-gc reviewed Aug 15, 2022

View reviewed changes

sam-gc approved these changes Aug 15, 2022

View reviewed changes

Update spy to use proper sinon chai syntax

c52f331

dwyfrequency merged commit dc7574e into master Aug 15, 2022

dwyfrequency deleted the jd-firefox-cross-browser branch August 15, 2022 20:03

dwyfrequency restored the jd-firefox-cross-browser branch August 16, 2022 18:56

firebase locked and limited conversation to collaborators Sep 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Firefox testing to Auth #6522

Add Firefox testing to Auth #6522

dwyfrequency commented Aug 9, 2022

changeset-bot bot commented Aug 9, 2022 •

edited

Loading

github-actions bot commented Aug 9, 2022 •

edited

Loading

google-oss-bot commented Aug 9, 2022 •

edited

Loading

`@firebase/database`

`@firebase/database-compat/standalone`

`bundle`

`firebase`

google-oss-bot commented Aug 9, 2022 •

edited

Loading

hsubox76 left a comment

hsubox76 Aug 12, 2022

dwyfrequency Aug 12, 2022

hsubox76 Aug 12, 2022

dwyfrequency Aug 12, 2022

hsubox76 Aug 12, 2022

dwyfrequency Aug 12, 2022

hsubox76 Aug 12, 2022

hsubox76 Aug 12, 2022

sam-gc left a comment

sam-gc Aug 15, 2022

sam-gc left a comment

Add Firefox testing to Auth #6522

Add Firefox testing to Auth #6522

Conversation

dwyfrequency commented Aug 9, 2022

changeset-bot bot commented Aug 9, 2022 • edited Loading

⚠️ No Changeset found

github-actions bot commented Aug 9, 2022 • edited Loading

Changeset File Check ✅

google-oss-bot commented Aug 9, 2022 • edited Loading

Size Report 1

Affected Products

@firebase/database

@firebase/database-compat/standalone

bundle

firebase

Test Logs

google-oss-bot commented Aug 9, 2022 • edited Loading

Size Analysis Report 1

Test Logs

hsubox76 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-gc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sam-gc left a comment

Choose a reason for hiding this comment

changeset-bot bot commented Aug 9, 2022 •

edited

Loading

github-actions bot commented Aug 9, 2022 •

edited

Loading

google-oss-bot commented Aug 9, 2022 •

edited

Loading

Size Report ¹

`@firebase/database`

`@firebase/database-compat/standalone`

`bundle`

`firebase`

google-oss-bot commented Aug 9, 2022 •

edited

Loading

Size Analysis Report ¹