Skip to content

Comment out the test that uses implemented NetworkInterface to accommodate dart sdk breaking change.#11659

Merged
auto-submit[bot] merged 2 commits intoflutter:mainfrom
aam:interface-address-dart-sdk-breaking-change
May 7, 2026
Merged

Comment out the test that uses implemented NetworkInterface to accommodate dart sdk breaking change.#11659
auto-submit[bot] merged 2 commits intoflutter:mainfrom
aam:interface-address-dart-sdk-breaking-change

Conversation

@aam
Copy link
Copy Markdown
Member

@aam aam commented May 6, 2026

This addresses dart sdk breaking change dart-lang/sdk#63216.

flutter/flutter#186155 tracks undoing this once dart rolls into flutter.

@flutter-dashboard flutter-dashboard Bot added the CICD Run CI/CD label May 6, 2026
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the multicast_dns tests to use InterfaceAddress instead of InternetAddress within the FakeNetworkInterface implementation. Feedback identifies a compilation error in the FakeInterfaceAddress class where the address property returns a String instead of an InternetAddress. Additionally, the reviewer noted that several methods were included that are not part of the InterfaceAddress interface and suggested returning null for the broadcast property to improve stability.

I am having trouble creating individual review comments. Click here to see my feedback.

packages/multicast_dns/test/client_test.dart (286-320)

high

The FakeInterfaceAddress implementation has a significant type error and includes methods not present in the InterfaceAddress interface:

  1. Type Error: InterfaceAddress.address must return an InternetAddress object, but your implementation returns a String (from _internetAddress.address). This will cause a compilation error.
  2. Extra Methods: isLinkLocal, isLoopback, isMulticast, and reverse() are members of InternetAddress, not InterfaceAddress. They should be removed to strictly adhere to the interface.
  3. Broadcast: Returning null for the broadcast property is generally safer than throwing an UnimplementedError if the property is accessed during interface enumeration.
class FakeInterfaceAddress implements InterfaceAddress {
  const FakeInterfaceAddress(this.address);

  @override
  final InternetAddress address;

  @override
  String get host => address.host;

  @override
  Uint8List get rawAddress => address.rawAddress;

  @override
  InternetAddressType get type => address.type;

  @override
  int get prefixLength => 0;

  @override
  InternetAddress? get broadcast => null;
}

@aam
Copy link
Copy Markdown
Member Author

aam commented May 6, 2026

@stuartmorgan-g you might know - how do you deal with dart sdk breaking changes that affect this repo?

This repo uses dart sdk from flutter, landing new dart sdk into flutter is impossible because the roll breaks these flutter packages, but then updating fluttter packages and rolling updated flutter packages together with roll of dart sdk is impossible because flutter packages uses old dart sdk(from flutter) so updating PR(like this one) fails.

@stuartmorgan-g
Copy link
Copy Markdown
Collaborator

how do you deal with dart sdk breaking changes that affect this repo?

Usually we don't; I'm having a hard time remembering a case where the Dart SDK broke us with a change that wasn't in a major Dart version.

There are a few problems, which have different solutions:

  1. The flutter tool actually uses this package, so there's a true circular dependency. This is bad, and I'm in the process of standing up flutter/core-packages precisely to fix issues like that (this package is slated to be moved as soon as I have the CI set up there). Luckily we can ignore this problem here since this is test-only.
  2. The analyzer run in flutter/flutter blocking the Dart->flutter/flutter roll can be addressed by temporarily making that test in flutter/flutter non-blocking to temporarily break that artificial cycle in our test infra. However, that will just kick the can down the road to:
  3. Once the roll lands in flutter/flutter, the flutter/flutter->flutter/packages roll will be blocked, because we analyze our packages on both stable and main.

We'll need a solution to 3, so if we can solve it in a way that fixes 2 we can avoid having to turn tests off and on in flutter/flutter.

I haven't dug into the exact details here; could we do something slightly gross with dynamic to make this compile for both versions? If not, given the limited scope in this particular instance I think my preference would be to just comment out this test, with a tracking issue to turn it back on once this breaking change reaches stable.

@github-actions github-actions Bot removed the CICD Run CI/CD label May 6, 2026
@aam
Copy link
Copy Markdown
Member Author

aam commented May 6, 2026

I haven't dug into the exact details here; could we do something slightly gross with dynamic to make this compile for both versions? If not, given the limited scope in this particular instance I think my preference would be to just comment out this test, with a tracking issue to turn it back on once this breaking change reaches stable.

thank you Stuart. I commented out the test with actual changes as comments so this can be uncommented and test restored as soon as dart sdk lands in flutter.

@aam aam changed the title Update FakeNetworkInterface to accommodate dart sdk breaking change. Comment out the test that uses implemented NetworkInterface to accommodate dart sdk breaking change. May 6, 2026
@aam aam added the CICD Run CI/CD label May 6, 2026
@iinozemtsev iinozemtsev added the autosubmit Merge PR when tree becomes green via auto submit App label May 7, 2026
@auto-submit auto-submit Bot merged commit 0411f1d into flutter:main May 7, 2026
84 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autosubmit Merge PR when tree becomes green via auto submit App CICD Run CI/CD p: multicast_dns

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants