diff --git a/README.md b/README.md index d0ffef9de6..4a56304e16 100644 --- a/README.md +++ b/README.md @@ -66,8 +66,58 @@ from any mobile devices you ran it on) when done. [download-zuliprc]: https://zulip.com/api/api-keys +### Tests + +You can run all our forms of tests with two commands: + +``` +$ flutter analyze +$ flutter test +``` + +Both should always pass, with no errors or warnings of any kind. + +The `flutter analyze` command runs the Dart analyzer, which performs +type-checking and linting. The `flutter test` command runs our +unit tests, located in the `test/` directory. + +Both commands accept a list of file or directory paths to operate +only on those files, and other options. + +When editing in an IDE, the IDE should give you the exact same feedback +as `flutter analyze` would. When editing a test file, the IDE can also +run individual tests for you. +See [upstream docs on `flutter test`][flutter-cookbook-unit-tests]. + +[flutter-cookbook-unit-tests]: https://docs.flutter.dev/cookbook/testing/unit/introduction + + ## Notes +### Writing tests + +For unit tests, we use [the `checks` package][package-checks]. +This is a new package from the Dart team, currently in preview, +which is [intended to replace][package-checks-migration] the +old `matcher` package. + +This means that if you see example test code elsewhere that +uses the `expect` function, we'd prefer to translate it into +something in terms of `check`. For help with that, +see the [`package:checks` migration guide][package-checks-migration] +and the package's [API docs][package-checks-api]. + +Because `package:checks` is still in preview, the Dart team is +open to feedback on the API to a degree that they won't be +after it reaches 1.0. So where we find rough edges, now is a +good time to [report them as issues][dart-test-tracker]. + +[package-checks]: https://pub.dev/packages/checks +[package-checks-api]: https://pub.dev/documentation/checks/latest/checks/checks-library.html +[package-checks-migration]: https://github.com/dart-lang/test/blob/master/pkgs/checks/doc/migrating_from_matcher.md +[dart-test-tracker]: https://github.com/dart-lang/test/issues + + ### Editing API types We support Zulip Server 4.0 and later. For API features added in @@ -96,7 +146,7 @@ To update the version bounds: * Update the lower bounds at `environment` in `pubspec.yaml` to the new versions, as seen in `flutter --version`. * Run `flutter pub get`, which will update `pubspec.lock`. -* Make a quick check that things work: `flutter analyze`, +* Make a quick check that things work: `flutter analyze && flutter test`, and do a quick smoke-test of the app. * Commit and push the changes in `pubspec.yaml` and `pubspec.lock`. diff --git a/lib/model/content.dart b/lib/model/content.dart index 50d4a938e4..3858d3e414 100644 --- a/lib/model/content.dart +++ b/lib/model/content.dart @@ -2,8 +2,15 @@ import 'package:flutter/foundation.dart'; import 'package:html/dom.dart' as dom; import 'package:html/parser.dart'; +// TODO: Implement ==/hashCode for all these classes where O(1), for testing/debugging +// (Skip them for classes containing lists.) +// TODO: Implement toString for all these classes, for testing/debugging; or +// perhaps Diagnosticable instead? + +// This should have no subclasses except the ones defined in this file. +// TODO mark as sealed: https://github.com/dart-lang/language/blob/a374667bc/accepted/future-releases/sealed-types/feature-specification.md @immutable -class ContentNode { +abstract class ContentNode { const ContentNode({this.debugHtmlNode}); final dom.Node? debugHtmlNode; @@ -28,6 +35,9 @@ class ZulipContent extends ContentNode { const ZulipContent({super.debugHtmlNode, required this.nodes}); final List nodes; + + @override + String toString() => '${objectRuntimeType(this, 'ZulipContent')}($nodes)'; } abstract class BlockContentNode extends ContentNode { @@ -45,6 +55,14 @@ class UnimplementedBlockContentNode extends BlockContentNode // A `br` element. class LineBreakNode extends BlockContentNode { const LineBreakNode({super.debugHtmlNode}); + + @override + bool operator ==(Object other) { + return other is LineBreakNode; + } + + @override + int get hashCode => 'LineBreakNode'.hashCode; } // A `p` element, or a place where the DOM tree logically wanted one. @@ -63,6 +81,12 @@ class ParagraphNode extends BlockContentNode { final bool wasImplicit; final List nodes; + + // No == or hashCode overrides; don't want to walk through [nodes] in + // an operation that looks cheap. + + @override + String toString() => '${objectRuntimeType(this, 'ParagraphNode')}(wasImplicit: $wasImplicit, $nodes)'; } enum ListStyle { ordered, unordered } @@ -122,6 +146,19 @@ class TextNode extends InlineContentNode { const TextNode(this.text, {super.debugHtmlNode}); final String text; + + @override + bool operator ==(Object other) { + return other is TextNode + && other.text == text; + } + + @override + int get hashCode => Object.hash('TextNode', text); + + // TODO encode unambiguously regardless of text contents + @override + String toString() => '${objectRuntimeType(this, 'TextNode')}(text: $text)'; } class LineBreakInlineNode extends InlineContentNode { diff --git a/pubspec.lock b/pubspec.lock index cf8a0e1513..8ff1657685 100644 --- a/pubspec.lock +++ b/pubspec.lock @@ -121,6 +121,14 @@ packages: url: "https://pub.dev" source: hosted version: "2.0.2" + checks: + dependency: "direct dev" + description: + name: checks + sha256: "028fb54c181e68edc3c891d7d45813937311e29c77609f513bb5025620637ffd" + url: "https://pub.dev" + source: hosted + version: "0.2.1" clock: dependency: transitive description: @@ -153,6 +161,14 @@ packages: url: "https://pub.dev" source: hosted version: "3.1.1" + coverage: + dependency: transitive + description: + name: coverage + sha256: "2fb815080e44a09b85e0f2ca8a820b15053982b2e714b59267719e8a9ff17097" + url: "https://pub.dev" + source: hosted + version: "1.6.3" crypto: dependency: transitive description: @@ -371,6 +387,14 @@ packages: url: "https://pub.dev" source: hosted version: "1.0.4" + node_preamble: + dependency: transitive + description: + name: node_preamble + sha256: "8ebdbaa3b96d5285d068f80772390d27c21e1fa10fb2df6627b1b9415043608d" + url: "https://pub.dev" + source: hosted + version: "2.0.1" package_config: dependency: transitive description: @@ -419,6 +443,22 @@ packages: url: "https://pub.dev" source: hosted version: "1.4.0" + shelf_packages_handler: + dependency: transitive + description: + name: shelf_packages_handler + sha256: aef74dc9195746a384843102142ab65b6a4735bb3beea791e63527b88cc83306 + url: "https://pub.dev" + source: hosted + version: "3.0.1" + shelf_static: + dependency: transitive + description: + name: shelf_static + sha256: e792b76b96a36d4a41b819da593aff4bdd413576b3ba6150df5d8d9996d2e74c + url: "https://pub.dev" + source: hosted + version: "1.1.1" shelf_web_socket: dependency: transitive description: @@ -448,6 +488,22 @@ packages: url: "https://pub.dev" source: hosted version: "1.3.3" + source_map_stack_trace: + dependency: transitive + description: + name: source_map_stack_trace + sha256: "84cf769ad83aa6bb61e0aa5a18e53aea683395f196a6f39c4c881fb90ed4f7ae" + url: "https://pub.dev" + source: hosted + version: "2.1.1" + source_maps: + dependency: transitive + description: + name: source_maps + sha256: "708b3f6b97248e5781f493b765c3337db11c5d2c81c3094f10904bfa8004c703" + url: "https://pub.dev" + source: hosted + version: "0.10.12" source_span: dependency: transitive description: @@ -496,6 +552,14 @@ packages: url: "https://pub.dev" source: hosted version: "1.2.1" + test: + dependency: "direct dev" + description: + name: test + sha256: "5301f54eb6fe945daa99bc8df6ece3f88b5ceaa6f996f250efdaaf63e22886be" + url: "https://pub.dev" + source: hosted + version: "1.23.1" test_api: dependency: transitive description: @@ -504,6 +568,14 @@ packages: url: "https://pub.dev" source: hosted version: "0.4.18" + test_core: + dependency: transitive + description: + name: test_core + sha256: d2e9240594b409565524802b84b7b39341da36dd6fd8e1660b53ad928ec3e9af + url: "https://pub.dev" + source: hosted + version: "0.4.24" timing: dependency: transitive description: @@ -528,6 +600,14 @@ packages: url: "https://pub.dev" source: hosted version: "2.1.4" + vm_service: + dependency: transitive + description: + name: vm_service + sha256: eb3cf3f45fc1500ae30481ac9ab788302fa5e8edc3f3eaddf183945ee93a8bf3 + url: "https://pub.dev" + source: hosted + version: "11.2.0" watcher: dependency: transitive description: @@ -544,6 +624,14 @@ packages: url: "https://pub.dev" source: hosted version: "2.3.0" + webkit_inspection_protocol: + dependency: transitive + description: + name: webkit_inspection_protocol + sha256: "67d3a8b6c79e1987d19d848b0892e582dbb0c66c57cc1fef58a177dd2aa2823d" + url: "https://pub.dev" + source: hosted + version: "1.2.0" yaml: dependency: transitive description: diff --git a/pubspec.yaml b/pubspec.yaml index f5f9bd285f..6895ba6589 100644 --- a/pubspec.yaml +++ b/pubspec.yaml @@ -59,6 +59,8 @@ dev_dependencies: json_serializable: ^6.5.4 build_runner: ^2.3.3 + test: ^1.23.1 + checks: ^0.2.1 # For information on the generic Dart part of this file, see the # following page: https://dart.dev/tools/pub/pubspec diff --git a/test/model/content_checks.dart b/test/model/content_checks.dart new file mode 100644 index 0000000000..402c43ae1d --- /dev/null +++ b/test/model/content_checks.dart @@ -0,0 +1,41 @@ +import 'package:checks/checks.dart'; +import 'package:zulip/model/content.dart'; + +extension ContentNodeChecks on Subject { + void equalsNode(ContentNode expected) { + if (expected is ZulipContent) { + isA() + .nodes.deepEquals(expected.nodes.map( + (e) => it()..isA().equalsNode(e))); + // A shame we need the dynamic `isA` there. This + // version hits a runtime type error: + // .nodes.deepEquals(expected.nodes.map( + // (e) => it()..equalsNode(e))); + // and with `it()` with no type argument, it doesn't type-check. + // TODO: report that as API feedback on deepEquals + } else if (expected is ParagraphNode) { + isA() + ..wasImplicit.equals(expected.wasImplicit) + ..nodes.deepEquals(expected.nodes.map( + (e) => it()..isA().equalsNode(e))); + } else { + // TODO handle remaining ContentNode subclasses that lack structural == + equals(expected); + } + } +} + +extension ZulipContentChecks on Subject { + Subject> get nodes => has((n) => n.nodes, 'nodes'); +} + +extension ParagraphNodeChecks on Subject { + Subject get wasImplicit => has((n) => n.wasImplicit, 'wasImplicit'); + Subject> get nodes => has((n) => n.nodes, 'nodes'); +} + +extension TextNodeChecks on Subject { + Subject get text => has((n) => n.text, 'text'); +} + +// TODO write similar extensions for the rest of the content node classes diff --git a/test/model/content_test.dart b/test/model/content_test.dart new file mode 100644 index 0000000000..75f33e8f4a --- /dev/null +++ b/test/model/content_test.dart @@ -0,0 +1,24 @@ +import 'package:checks/checks.dart'; +import 'package:test/scaffolding.dart'; +import 'package:zulip/model/content.dart'; + +import 'content_checks.dart'; + +void main() { + test('parse a plain-text paragraph', () { + check(parseContent('

hello world

')) + .equalsNode(const ZulipContent(nodes: [ + ParagraphNode(nodes: [TextNode('hello world')]), + ])); + }); + + test('parse two plain-text paragraphs', () { + check(parseContent('

hello

world

')) + .equalsNode(const ZulipContent(nodes: [ + ParagraphNode(nodes: [TextNode('hello')]), + ParagraphNode(nodes: [TextNode('world')]), + ])); + }); + + // TODO write more tests for this code +}