Skip to content

test(storage): ensure both standard localhost and Android localhost are handled #17484

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 1 commit into from
Jul 8, 2025

Conversation

EchoEllet
Copy link
Contributor

@EchoEllet EchoEllet commented Jul 2, 2025

Description

Add unit tests to ensure that partsFromHttpUrl does not return null for localhost URLs on all platforms.

The PR #7003 fixed an issue where refFromUrl returns null when the URL is localhost, which is required when connecting to the Firebase emulator of the machine. While that fix works on all platforms that use the standard localhost, it does not work on Android emulators, where the only way to connect to the machine of the emulator is by using the 127.0.0.1 address, so #12047 patched it to work on Android as well.

The PR #12047 was sent by me, and I just came across this code. I improved it a bit by:

  • Adding unit tests to handle both localhost and 127.0.0.1, so adding these two unit tests while removing these two PRs will cause the tests to fail as expected.
  • Refactoring the related code to improve the readability and avoid duplicating localhost and 127.0.0.1

Related Issues

Emulators vs Firebase Emulators?

It might be confusing, but by emulators I mean Android and iOS emulators, and by Firebase emulators I mean Firebase Local Emulator Suite.

To test and use Firebase fully, only the development machine without a real Android/iOS device and a Firebase project, the emulators are used to run the mobile app, and Firebase emulators are used on the development machine for local hosting, so the apps can connect to them locally.

All platforms, including iOS simulators, desktop, and web, need localhost to connect to Firebase emulators, while Android emulators need 127.0.0.1 (special case).

Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See Contributor Guide).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (comments with //).
  • The analyzer (melos run analyze) does not report any problems on my PR.
  • I read and followed the Flutter Style Guide.
  • I signed the CLA.
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

  • Yes, this is a breaking change.
  • No, this is not a breaking change.

@EchoEllet EchoEllet changed the title test: ensure localhost and 10.0.2.2 are handled test(storage): ensure localhost and 10.0.2.2 are handled Jul 2, 2025
@EchoEllet EchoEllet force-pushed the refactor/pr-12047 branch from daf0042 to aa312c6 Compare July 2, 2025 22:04
@EchoEllet EchoEllet changed the title test(storage): ensure localhost and 10.0.2.2 are handled test(storage): ensure both standard localhost and Android localhost are handled Jul 2, 2025
@SelaseKay
Copy link
Contributor

Hi @EchoEllet, LGTM!

Copy link
Contributor

@MichaelVerdon MichaelVerdon left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks.

@SelaseKay SelaseKay merged commit c56eccf into firebase:main Jul 8, 2025
24 of 29 checks passed
@EchoEllet EchoEllet deleted the refactor/pr-12047 branch July 8, 2025 12:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants