Skip to content
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

Add svg finder #880

Merged
merged 5 commits into from
Apr 11, 2023
Merged

Conversation

SimonWeidemann
Copy link
Contributor

This pull request adds a test helper to find svgs in widget tests.
The implementation is similar to the default find.image function.

pubspec.yaml Outdated
Comment on lines 10 to 11
flutter_test:
sdk: flutter
Copy link
Owner

Choose a reason for hiding this comment

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

This has to get moved to another package. We can't have the runtime depending on flutter_test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry for the late response.
I see what you mean.
How should we go further?
Should I create a separate repository or should we integrate the package in this one?
For example like this:

flutter_svg
│ 
├──.github
├──.gitignore
├──READMED.md
├──LICENSE
├──......
└─packages
       │ 
       ├── flutter_svg
       │         ├── example
       │         ├── lib
       │         ├── test
       │         ├── README.md
       │         ├── ....
       │         └── pubspec.yaml
       │ 
       └── flutter_svg_test
                   ├── example
                   ├── lib
                   ├── test
                   ├── README.md
                   ├── ....
                   └── pubspec.yaml

I am willing to make this work, but you probably need to guide me a bit.

I can add some more finder like the following one, so the package is not that small.

/// Finds widgets created by [SvgPicture.asset] with the [path] argument.
///
/// ## Sample code
/// ```dart
/// expect(svgAssetWithPath('assets/asset_name.svg'), findsOneWidget);
/// ```
/// This will match one [SvgPicture.asset] with the [path] 'assets/asset_name.svg'.
///
/// If the `skipOffstage` argument is true (the default), then this skips
/// nodes that are [Offstage] or that are from inactive [Route]s.
Finder svgAssetWithPath(String path, {bool skipOffstage = true}) {
  return _SvgAssetWithPathFinder(svgPath: path, skipOffstage: skipOffstage);
}

class _SvgAssetWithPathFinder extends MatchFinder {
  _SvgAssetWithPathFinder({required String svgPath, super.skipOffstage})
      : _svgPath = svgPath;

  final String _svgPath;

  @override
  String get description => "Path: '$_svgPath' and created by SvgPicture.asset";

  @override
  bool matches(Element candidate) {
    var result = false;
    final widget = candidate.widget;
    if (widget is SvgPicture) {
      final bytesLoader = widget.bytesLoader;
      if (bytesLoader is SvgAssetLoader) {
        result = bytesLoader.assetName == _svgPath;
      }
    }
    return result;
  }
}

Copy link
Owner

Choose a reason for hiding this comment

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

Yes, this needs to be its own package, which the repo isn't currently structured well for. I think your proposed structure is fine.

@SimonWeidemann SimonWeidemann requested a review from dnfield March 30, 2023 11:24
@SimonWeidemann SimonWeidemann force-pushed the feature/add-svg-finder branch 12 times, most recently from 035139e to f8b4f69 Compare April 7, 2023 16:48
Copy link
Owner

Choose a reason for hiding this comment

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

Please update this file with a description of what this package is and how to use it.

Copy link
Owner

Choose a reason for hiding this comment

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

I think this should be removed?

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think this file is necessary

Copy link
Owner

Choose a reason for hiding this comment

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

I think this is duplicated from the other package and isn't referred to anywhere, can it be removed?

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 use this file in two SvgFileLoader tests.

@dnfield
Copy link
Owner

dnfield commented Apr 10, 2023

This looks close. It'll need a conflict resolution. Thanks!

@SimonWeidemann SimonWeidemann force-pushed the feature/add-svg-finder branch from f8b4f69 to e10d531 Compare April 10, 2023 22:14
@SimonWeidemann
Copy link
Contributor Author

SimonWeidemann commented Apr 10, 2023

I worked through the review comments.
Now the repository has a readme and also each package .
I thought this would be more suitable.

@SimonWeidemann SimonWeidemann requested a review from dnfield April 10, 2023 22:29
@SimonWeidemann SimonWeidemann force-pushed the feature/add-svg-finder branch from e10d531 to 98bf349 Compare April 10, 2023 22:32
final SvgPicture asset = SvgPicture.network('svg.dart');
await widgetTester.pumpWidget(asset);

expect(find.svg(asset.bytesLoader), findsOneWidget);
Copy link
Owner

Choose a reason for hiding this comment

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

So looking at this, it surprises me that we're not just passing in the SvgPicture, i.e. find.svg(asset).

Sorry I didn't see this until now. This is looking pretty close though.

Copy link
Contributor Author

@SimonWeidemann SimonWeidemann Apr 11, 2023

Choose a reason for hiding this comment

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

If I adjust the finder accordingly:

        Finder svg(SvgPicture svg, {bool skipOffstage = true}) {
           return _SvgFinder(svg, skipOffstage: skipOffstage);
         }

      class _SvgFinder extends MatchFinder {
        _SvgFinder(this._svg, {super.skipOffstage});
      
         final SvgPicture _svg;
      
         @override
         String get description => "svg: '$_svg'";
      
         @override
         bool matches(Element candidate) {
           return candidate.widget == _svg;
         }
       }

For me the following test should pass:

      testWidgets('asset svg', (WidgetTester widgetTester) async {
        final SvgPicture asset = SvgPicture.asset( 'test/flutter_logo.svg');
        final SvgPicture asset2 = SvgPicture.asset('test/flutter_logo.svg');
        await widgetTester.pumpWidget(asset);
        expect(find.svg(asset2), findsOneWidget);
      });

But unfortunately the test does not pass, because the equals method compares the object not the fields.
If I do this by hand my finder would look like this:

        Finder svg(SvgPicture svg, {bool skipOffstage = true}) {
           return _SvgFinder(svg, skipOffstage: skipOffstage);
         }

      class _SvgFinder extends MatchFinder {
        _SvgFinder(this._svg, {super.skipOffstage});
      
         final SvgPicture _svg;
      
         @override
         String get description => "svg: '$_svg'";
      
         @override
         bool matches(Element candidate) {
           return  widget is SvgPicture &&
                      _svg.runtimeType == widget.runtimeType &&
                     _svg.width == widget.width &&
                     _svg.height == widget.height &&
                     _svg.fit == widget.fit &&
                     _svg.alignment == widget.alignment &&
                     _svg.bytesLoader == widget.bytesLoader &&
                     _svg.placeholderBuilder == widget.placeholderBuilder &&
                     _svg.matchTextDirection == widget.matchTextDirection &&
                     _svg.allowDrawingOutsideViewBox == widget.allowDrawingOutsideViewBox &&
                     _svg.semanticsLabel == widget.semanticsLabel &&
                     _svg.excludeFromSemantics == widget.excludeFromSemantics &&
                     _svg.clipBehavior == widget.clipBehavior &&
                     _svg.colorFilter == widget.colorFilter &&
                     _svg.theme == widget.theme;
         }
       }

While writing the test helper I orientated on the find.text and find.image methods from the flutter library.
In both they compare just with a subset of the fields. In find.text with the string and infind.image
with the image provider.
I think this was done because otherwise you would have to declare all fields in the test.
Sometimes it is just needed to locate a widget and afterwards compare different aspects, with expect.
Also sometimes it would be really hard to compare objects during an animation, where for example the double value has a huge amount of decimal place.

I would go with the current implementation.

Thanks for the two commits!

Copy link
Owner

Choose a reason for hiding this comment

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

Ahh interesting. I would have assumed the impelementation would be something like return widget is SvgPicture && _svg.bytesLoader = widget.bytesLoader, but perhaps this makes more sense. If that's how this works for Image though I think it makes sense.

@dnfield dnfield merged commit fd85e3c into dnfield:master Apr 11, 2023
@dnfield
Copy link
Owner

dnfield commented Apr 11, 2023

Note to self: rolling this into google is going to require updates to the copy scripts

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.

2 participants