ci(passkit_ui): add workflow #26
Conversation
marcossevilla
added
the
package: passkit_ui
You can take a look at this part of the docs 👍
Yeah I think that would be super useful |
There was a problem hiding this comment.
Why don't you write golden image tests? The widgets do not have state and are non-interactive, so a golden image test would cover probably even more lines of code while being easier to write and maintain:
void main() {
// Arrange
testWidgets('boarding pass gets rendered correctly', (WidgetTester tester) async {
final bytes = await File('test/sample_passes/BoardingPass.pkpass').readAsBytes();
final pkPass = PkPass.fromBytes(bytes);
// Act
await tester.pumpWidget(MaterialApp(home: PkPassWidget(pass: pkPass)));
// Assert
await expectLater(find.byType(PkPassWidget), matchesGoldenFile('boarding_pass.png'));
});
}To test the various branches (e.g. for choosing the resolution appropriate image), you just supply different pkpass files. Assuming the tests are exhaustive, each uncovered line is dead code and can be removed. This also keeps basically of the code an implementation detail and it just tests the actual API surface of the library. That's also why PkPassWidget is the only exposed class from the library (yet). That way we can change the whole implementation, without breaking users nor tests, while still having a high confidence in the code being correct, since it always looks correct thanks to the golden images.
A concrete example for this implementation detail argument is that, in particular, I view the PkPassTheme class as an implementation detail. It should probably be an individual ThemeExtension per pass type instead (different pass types have different font styles for example). I don't really want to rewrite the tests you just wrote for the PkPassTheme, but I really want to keep the look (aka the golden image) the same.
That being said, I very much appreciate your effort :D
| extension PkPassImageX on PkPassImage { | ||
| Uint8List forCorrectPixelRatio(double devicePixelRatio) { | ||
| // It just looks better when using images a step higher. | ||
| return fromMultiplier(devicePixelRatio.toInt() + 1); | ||
| } | ||
| } |
There was a problem hiding this comment.
While I understand your reason for changing this, I think it's more important to make it easy to use here, especially now that you made it public facing. After all, that's what libraries for, to make the life of the consumer of the library easier. Also, a pass has at max like 3 or 4 images and one or three calls to the MediaQuery isn't really anything that makes or breaks the performance.
By keeping the BuildContext as parameter, we can also change how the correct image is retrieved later ta a different property of the MediaQuery instead, so it future-proofes the API in a way that doesn't break consumers.
Keeping the BuildContext as a parameter also makes the API more Flutter-like, since Flutter also always just takes a context as a parameter.
If it was just an internal API I wouldn't mind this change at all though.
There was a problem hiding this comment.
BuildContext shouldn't be a parameter for external methods, it should be used exclusively on the widgets' build method to avoid passing the whole context object as a function parameter and also causing context issues. It's even easier to test methods by passing a devicePixelRatio in this case than a mock build context. I think breaking changes are inevitable on packages and in this case it's a small one so it shouldn't be an issue!
| PkTextAlignment.natural when textDirection == TextDirection.ltr => | ||
| TextAlign.left, | ||
| PkTextAlignment.natural when textDirection == TextDirection.rtl => | ||
| TextAlign.right, |
| import 'package:passkit/passkit.dart'; | ||
|
|
||
| extension PkTextAlignmentX on PkTextAlignment { | ||
| TextAlign toFlutterTextAlign({TextDirection? textDirection}) { |
There was a problem hiding this comment.
Same as above. If it's public facing, it should take a context instead.
| flutter_version: 3.22.2 | ||
| working_directory: passkit_ui | ||
| # TODO: Increase minimum coverage to 100% gradually. | ||
| min_coverage: 50 |
There was a problem hiding this comment.
That's already higher than I expected :D
passkit_ui/lib/passkit_ui.dart
Outdated
| export 'src/widgets/widgets.dart'; | ||
| export 'src/extensions/extensions.dart'; | ||
| export 'src/theme/theme.dart'; | ||
| export 'src/boarding_pass.dart'; | ||
| export 'src/coupon.dart'; | ||
| export 'src/event_ticket.dart'; | ||
| export 'src/generic.dart'; | ||
| export 'src/store_card.dart'; |
There was a problem hiding this comment.
I would, at least for now, keep the PkPassWidget as the only exposed class. That makes iterating a bit easier, while ensuring that no consumer of the library gets broken due to API changes.
In particular, the PkPassTheme is something that's not yet ready to be public facing, since it probably should be a ThemeExtension instead.
|
Merging this but keeping in mind:
|
No description provided.