fix(firestore, web): Propagate FirebaseException properly, fix `mer…

…geFields` bug, fix `bytesLoaded` different type under different conditions (#12334)
firebase · Feb 16, 2024 · fdde75b · fdde75b
1 parent c42b57c
commit fdde75b
Show file tree

Hide file tree

Showing 11 changed files with 65 additions and 126 deletions.
diff --git a/.github/workflows/e2e_tests.yaml b/.github/workflows/e2e_tests.yaml
@@ -237,6 +237,8 @@ jobs:
     runs-on: macos-latest
     timeout-minutes: 15
     strategy:
+      # Temp measure for testing web tests. Please remove as soon as e2e tests are stable.
+      fail-fast: false
       matrix:
         working_directory:
           ['tests', 'packages/cloud_firestore/cloud_firestore/example']

diff --git a/packages/_flutterfire_internals/lib/_flutterfire_internals.dart b/packages/_flutterfire_internals/lib/_flutterfire_internals.dart
@@ -82,7 +82,9 @@ bool _testException(Object? objectException) {
   final exception = objectException! as core_interop.JSError;
   // ignore: unnecessary_cast
   final message = exception.message as String;
-  return message.contains('Firebase');
+  // Firestore web does not contain `Firebase` in the message so we check the exception itself.
+  return message.contains('Firebase') ||
+      exception.toString().contains('FirebaseError');
 }
 
 /// Transforms internal errors in something more readable for end-users.

diff --git a/...ages/cloud_firestore/cloud_firestore/example/integration_test/document_reference_e2e.dart b/...ages/cloud_firestore/cloud_firestore/example/integration_test/document_reference_e2e.dart
@@ -172,8 +172,6 @@ void runDocumentReferenceTests() {
           fail('Should have thrown a [FirebaseException]');
         });
       },
-      // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
-      skip: kIsWeb,
     );
 
     group('DocumentReference.delete()', () {
@@ -255,9 +253,7 @@ void runDocumentReferenceTests() {
             return;
           }
           fail('Should have thrown a [FirebaseException]');
-          // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
         },
-        skip: kIsWeb,
       );
     });
 
@@ -320,7 +316,6 @@ void runDocumentReferenceTests() {
             equals({'foo': 'bar', 'bar': 456, 'baz': 'foo'}),
           );
         },
-        skip: kIsWeb,
       );
 
       testWidgets(
@@ -340,9 +335,7 @@ void runDocumentReferenceTests() {
             return;
           }
           fail('Should have thrown a [FirebaseException]');
-          // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
         },
-        skip: kIsWeb,
       );
 
       testWidgets('set and return all possible datatypes', (_) async {
@@ -513,8 +506,6 @@ void runDocumentReferenceTests() {
             );
           }
         },
-        // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
-        skip: kIsWeb,
       );
     });
 

diff --git a/packages/cloud_firestore/cloud_firestore/example/integration_test/load_bundle_e2e.dart b/packages/cloud_firestore/cloud_firestore/example/integration_test/load_bundle_e2e.dart
@@ -97,9 +97,7 @@ void runLoadBundleTests() {
                   .having((e) => e.code, 'code', 'load-bundle-error'),
             ),
           );
-          // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
         },
-        skip: kIsWeb,
       );
 
       testWidgets(
@@ -181,16 +179,13 @@ void runLoadBundleTests() {
               'wrong-name',
               options: const GetOptions(source: Source.cache),
             ),
-            // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
-            // expect(error, isA<FirebaseException>());
             throwsA(
               isA<FirebaseException>()
                   .having((e) => e.code, 'code', 'non-existent-named-query'),
             ),
           );
         },
-        // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
-        skip: kIsWeb || defaultTargetPlatform == TargetPlatform.windows,
+        skip: defaultTargetPlatform == TargetPlatform.windows,
       );
     });
 
@@ -241,8 +236,7 @@ void runLoadBundleTests() {
             ),
           );
         },
-        // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
-        skip: kIsWeb || defaultTargetPlatform == TargetPlatform.windows,
+        skip: defaultTargetPlatform == TargetPlatform.windows,
       );
     });
   });

diff --git a/packages/cloud_firestore/cloud_firestore/example/integration_test/query_e2e.dart b/packages/cloud_firestore/cloud_firestore/example/integration_test/query_e2e.dart
@@ -7,7 +7,6 @@ import 'dart:math';
 
 import 'package:cloud_firestore/cloud_firestore.dart';
 import 'package:firebase_core/firebase_core.dart';
-import 'package:flutter/foundation.dart';
 import 'package:flutter_test/flutter_test.dart';
 
 void runQueryTests() {
@@ -210,8 +209,6 @@ void runQueryTests() {
           }
           fail('Should have thrown a [FirebaseException]');
         },
-        // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
-        skip: kIsWeb,
       );
 
       testWidgets(
@@ -234,8 +231,6 @@ void runQueryTests() {
             );
           }
         },
-        // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
-        skip: kIsWeb,
       );
     });
 
@@ -378,9 +373,7 @@ void runQueryTests() {
           }
 
           fail('Should have thrown a [FirebaseException]');
-          // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
         },
-        skip: kIsWeb,
       );
     });
 
@@ -2221,8 +2214,8 @@ void runQueryTests() {
           CollectionReference<Map<String, dynamic>> collection =
               await initializeTest('multiple-conjunctive-queries');
 
-          try {
-            await collection
+          await expectLater(
+            collection
                 .where(
                   Filter.and(
                     Filter('rating1', isEqualTo: 3.8),
@@ -2259,22 +2252,13 @@ void runQueryTests() {
                   ),
                 )
                 .orderBy('rating1', descending: true)
-                .get();
-          } catch (e) {
-            expect(
-              (e as FirebaseException)
-                      .message!
-                      .contains('Client specified an invalid argument.') ||
-                  e.message!.contains(
-                    'An error occurred while parsing query arguments',
-                  ),
-              isTrue,
-            );
-            expect(e, isA<FirebaseException>());
-          }
+                .get(),
+            throwsA(
+              isA<FirebaseException>()
+                  .having((e) => e.code, 'code', 'invalid-argument'),
+            ),
+          );
         },
-        // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
-        skip: kIsWeb,
       );
 
       testWidgets('allow multiple disjunctive queries', (_) async {
@@ -2398,8 +2382,8 @@ void runQueryTests() {
           CollectionReference<Map<String, dynamic>> collection =
               await initializeTest('multiple-disjunctive-queries');
 
-          try {
-            await collection
+          await expectLater(
+            collection
                 .where(
                   Filter.or(
                     Filter('rating', isEqualTo: 3.8),
@@ -2442,22 +2426,13 @@ void runQueryTests() {
                   ),
                 )
                 .orderBy('rating', descending: true)
-                .get();
-          } catch (e) {
-            expect(
-              (e as FirebaseException)
-                      .message!
-                      .contains('Client specified an invalid argument.') ||
-                  e.message!.contains(
-                    'An error occurred while parsing query arguments',
-                  ),
-              isTrue,
-            );
-            expect(e, isA<FirebaseException>());
-          }
+                .get(),
+            throwsA(
+              isA<FirebaseException>()
+                  .having((e) => e.code, 'code', 'invalid-argument'),
+            ),
+          );
         },
-        // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
-        skip: kIsWeb,
       );
 
       testWidgets(

diff --git a/packages/cloud_firestore/cloud_firestore/example/integration_test/second_app_e2e.dart b/packages/cloud_firestore/cloud_firestore/example/integration_test/second_app_e2e.dart
@@ -47,8 +47,7 @@ void runSecondAppTests() {
               ),
             );
           },
-          // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
-          skip: kIsWeb || defaultTargetPlatform == TargetPlatform.windows,
+          skip: defaultTargetPlatform == TargetPlatform.windows,
         );
       },
     );

diff --git a/packages/cloud_firestore/cloud_firestore/example/integration_test/second_database.dart b/packages/cloud_firestore/cloud_firestore/example/integration_test/second_database.dart
@@ -49,10 +49,11 @@ void runSecondDatabaseTests() {
       );
       QuerySnapshot<Map<String, dynamic>> snapshot = await collection.get();
 
-      await Future.forEach(snapshot.docs,
-          (QueryDocumentSnapshot<Map<String, dynamic>> documentSnapshot) {
+      List<Future> deleteFutures = snapshot.docs.map((documentSnapshot) {
         return documentSnapshot.reference.delete();
-      });
+      }).toList();
+
+      await Future.wait(deleteFutures);
       return collection;
     }
 
@@ -185,8 +186,6 @@ void runSecondDatabaseTests() {
           }
           fail('Should have thrown a [FirebaseException]');
         },
-        // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
-        skip: kIsWeb,
       );
     });
 
@@ -332,9 +331,7 @@ void runSecondDatabaseTests() {
           }
 
           fail('Should have thrown a [FirebaseException]');
-          // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
         },
-        skip: kIsWeb,
       );
     });
 
@@ -1772,8 +1769,8 @@ void runSecondDatabaseTests() {
           CollectionReference<Map<String, dynamic>> collection =
               await initializeTest('multiple-conjunctive-queries');
 
-          try {
-            await collection
+          await expectLater(
+            collection
                 .where(
                   Filter.and(
                     Filter('rating1', isEqualTo: 3.8),
@@ -1810,22 +1807,13 @@ void runSecondDatabaseTests() {
                   ),
                 )
                 .orderBy('rating1', descending: true)
-                .get();
-          } catch (e) {
-            expect(
-              (e as FirebaseException)
-                      .message!
-                      .contains('Client specified an invalid argument.') ||
-                  e.message!.contains(
-                    'An error occurred while parsing query arguments',
-                  ),
-              isTrue,
-            );
-            expect(e, isA<FirebaseException>());
-          }
+                .get(),
+            throwsA(
+              isA<FirebaseException>()
+                  .having((e) => e.code, 'code', 'invalid-argument'),
+            ),
+          );
         },
-        // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
-        skip: kIsWeb,
       );
 
       testWidgets(
@@ -1834,8 +1822,8 @@ void runSecondDatabaseTests() {
           CollectionReference<Map<String, dynamic>> collection =
               await initializeTest('multiple-disjunctive-queries');
 
-          try {
-            await collection
+          await expectLater(
+            collection
                 .where(
                   Filter.or(
                     Filter('rating', isEqualTo: 3.8),
@@ -1878,22 +1866,13 @@ void runSecondDatabaseTests() {
                   ),
                 )
                 .orderBy('rating', descending: true)
-                .get();
-          } catch (e) {
-            expect(
-              (e as FirebaseException)
-                      .message!
-                      .contains('Client specified an invalid argument.') ||
-                  e.message!.contains(
-                    'An error occurred while parsing query arguments',
-                  ),
-              isTrue,
-            );
-            expect(e, isA<FirebaseException>());
-          }
+                .get(),
+            throwsA(
+              isA<FirebaseException>()
+                  .having((e) => e.code, 'code', 'invalid-argument'),
+            ),
+          );
         },
-        // This will fail until this is resolved: https://github.com/dart-lang/sdk/issues/52572
-        skip: kIsWeb,
       );
 
       testWidgets('isEqualTo filter', (_) async {

diff --git a/packages/cloud_firestore/cloud_firestore/example/integration_test/write_batch_e2e.dart b/packages/cloud_firestore/cloud_firestore/example/integration_test/write_batch_e2e.dart
@@ -3,7 +3,6 @@
 // BSD-style license that can be found in the LICENSE file.
 
 import 'package:cloud_firestore/cloud_firestore.dart';
-import 'package:flutter/foundation.dart';
 import 'package:flutter_test/flutter_test.dart';
 
 void runWriteBatchTests() {
@@ -100,14 +99,11 @@ void runWriteBatchTests() {
       batch.update(doc3, <String, dynamic>{'bar': 'ben'});
       batch.set(doc4, <String, dynamic>{'bar': 'ben'}, SetOptions(merge: true));
 
-      // TODO(ehesp): firebase-dart does not support mergeFields
-      if (!kIsWeb) {
-        batch.set(
-          doc5,
-          <String, dynamic>{'bar': 'ben'},
-          SetOptions(mergeFields: ['bar']),
-        );
-      }
+      batch.set(
+        doc5,
+        <String, dynamic>{'bar': 'ben'},
+        SetOptions(mergeFields: ['bar']),
+      );
 
       await batch.commit();
 
@@ -127,14 +123,11 @@ void runWriteBatchTests() {
         snapshot.docs.firstWhere((doc) => doc.id == 'doc4').data(),
         equals(<String, dynamic>{'foo': 'bar', 'bar': 'ben'}),
       );
-      // ignore: todo
-      // TODO(ehesp): firebase-dart does not support mergeFields
-      if (!kIsWeb) {
-        expect(
-          snapshot.docs.firstWhere((doc) => doc.id == 'doc5').data(),
-          equals(<String, dynamic>{'foo': 'bar', 'bar': 'ben'}),
-        );
-      }
+
+      expect(
+        snapshot.docs.firstWhere((doc) => doc.id == 'doc5').data(),
+        equals(<String, dynamic>{'foo': 'bar', 'bar': 'ben'}),
+      );
     });
   });
 }