From 7d760b1316e698e0f3df8aad715c9a75e3794462 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 27 Jun 2023 18:26:39 -0700 Subject: [PATCH 1/2] content [nfc]: Fix up docs formatting a bit --- lib/model/content.dart | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/lib/model/content.dart b/lib/model/content.dart index 1dd237b3f7..f7971ecfba 100644 --- a/lib/model/content.dart +++ b/lib/model/content.dart @@ -92,7 +92,7 @@ class ZulipContent extends ContentNode { /// /// Generally these correspond to HTML elements which in the Zulip web client /// are laid out as block-level boxes, in a block formatting context: -/// https://developer.mozilla.org/en-US/docs/Web/CSS/CSS_flow_layout/Block_and_inline_layout_in_normal_flow +/// /// /// Almost all nodes are either a [BlockContentNode] or an [InlineContentNode]. abstract class BlockContentNode extends ContentNode { @@ -143,14 +143,14 @@ class LineBreakNode extends BlockContentNode { int get hashCode => 'LineBreakNode'.hashCode; } -// A `p` element, or a place where the DOM tree logically wanted one. -// -// We synthesize these in the absence of an actual `p` element in cases where -// there's inline content (like [dom.Text] nodes, links, or spans) in a context -// where block content can also appear (like inside a `li`.) These are marked -// with [wasImplicit]. -// -// See also [parseImplicitParagraphBlockContentList]. +/// A `p` element, or a place where the DOM tree logically wanted one. +/// +/// We synthesize these in the absence of an actual `p` element in cases where +/// there's inline content (like [dom.Text] nodes, links, or spans) in a context +/// where block content can also appear (like inside a `li`.) These are marked +/// with [wasImplicit]. +/// +/// See also [parseImplicitParagraphBlockContentList]. class ParagraphNode extends BlockInlineContainerNode { const ParagraphNode( {super.debugHtmlNode, required super.nodes, this.wasImplicit = false}); From 6f9df45c118199558fa92e43ae5266600d4f7318 Mon Sep 17 00:00:00 2001 From: Greg Price Date: Tue, 27 Jun 2023 17:29:15 -0700 Subject: [PATCH 2/2] content test: Compare parse trees via Diagnosticable This makes for significantly cleaner and more informative output when a test fails (resolving all the TODO-comments), by taking more control of how the comparison is done. At the same time, it makes for less boilerplate and fewer places to update when adding more features in the ContentNode parse trees. Effectively it deduplicates some of the boilerplate, by reusing for comparing the trees the metadata that we're already prepared to generate for printing them out when a comparison fails. --- lib/model/content.dart | 17 ++-- test/model/content_checks.dart | 142 +++++++++++---------------------- 2 files changed, 57 insertions(+), 102 deletions(-) diff --git a/lib/model/content.dart b/lib/model/content.dart index f7971ecfba..88afbf9114 100644 --- a/lib/model/content.dart +++ b/lib/model/content.dart @@ -17,12 +17,16 @@ import 'package:html/parser.dart'; /// * Override [debugDescribeChildren] and/or [debugFillProperties] /// to report all the data attached to the node, for debugging. /// See docs: https://api.flutter.dev/flutter/foundation/Diagnosticable/debugFillProperties.html +/// We also rely on these for comparing actual to expected in tests. /// -/// When modifying subclasses: -/// * Always check the following places to see if they need a matching update: -/// * [==] and [hashCode], if overridden. -/// * [debugFillProperties] and/or [debugDescribeChildren], if present. -/// * `equalsNode` in test/model/content_checks.dart . +/// When modifying subclasses, always check the following places +/// to see if they need a matching update: +/// * [==] and [hashCode], if overridden. +/// * [debugFillProperties] and/or [debugDescribeChildren]. +/// +/// In particular, a newly-added field typically must be added in +/// [debugFillProperties]. Otherwise tests will not examine the new field, +/// and will not spot discrepancies there. @immutable sealed class ContentNode extends DiagnosticableTree { const ContentNode({this.debugHtmlNode}); @@ -42,9 +46,6 @@ sealed class ContentNode extends DiagnosticableTree { @override String toString({DiagnosticLevel minLevel = DiagnosticLevel.info}) { - // TODO(checks): Better integrate package:checks with Diagnosticable, for - // better interaction in output indentation. - // (See also comment at equalsNode, with related improvements.) String? result; assert(() { result = toStringDeep(minLevel: minLevel); diff --git a/test/model/content_checks.dart b/test/model/content_checks.dart index d0412d515e..c9f38af1a7 100644 --- a/test/model/content_checks.dart +++ b/test/model/content_checks.dart @@ -1,108 +1,62 @@ -import 'package:checks/checks.dart'; +import 'package:checks/context.dart'; +import 'package:flutter/foundation.dart'; import 'package:zulip/model/content.dart'; extension ContentNodeChecks on Subject { void equalsNode(ContentNode expected) { - // TODO: Make equalsNode output clearer on failure, applying Diagnosticable. - // In particular (a) show the top-level expected node in one piece - // (as well as the actual); (a') ideally, suppress on the "expected" side - // the various predicates below, which should be redundant with just - // the expected node; (b) show expected for the specific `equals` leaf. - // See also comment on [ContentNode.toString]. - if (expected is ZulipContent) { - isA() - .nodes.equalsNodes(expected.nodes); - } else if (expected is UnimplementedBlockContentNode) { - isA() - .debugHtmlText.equals(expected.debugHtmlText); - } else if (expected is ParagraphNode) { - isA() - ..wasImplicit.equals(expected.wasImplicit) - ..nodes.equalsNodes(expected.nodes); - } else if (expected is HeadingNode) { - isA() - ..level.equals(expected.level) - ..nodes.equalsNodes(expected.nodes); - } else if (expected is ListNode) { - isA() - ..style.equals(expected.style) - ..items.deepEquals(expected.items.map( - (item) => it()..isA>().equalsNodes(item))); - } else if (expected is QuotationNode) { - isA() - .nodes.equalsNodes(expected.nodes); - } else if (expected is UnimplementedInlineContentNode) { - isA() - .debugHtmlText.equals(expected.debugHtmlText); - } else if (expected is StrongNode) { - isA() - .nodes.equalsNodes(expected.nodes); - } else if (expected is EmphasisNode) { - isA() - .nodes.equalsNodes(expected.nodes); - } else if (expected is InlineCodeNode) { - isA() - .nodes.equalsNodes(expected.nodes); - } else if (expected is LinkNode) { - isA() - .nodes.equalsNodes(expected.nodes); - } else if (expected is UserMentionNode) { - isA() - .nodes.equalsNodes(expected.nodes); - } else { - // The remaining node types have structural `==`. Use that. - equals(expected); - } + return context.expect(() => prefixFirst('equals ', literal(expected)), (actual) { + final which = _compareDiagnosticsNodes( + actual.toDiagnosticsNode(), expected.toDiagnosticsNode()); + return which == null ? null : Rejection(which: [ + 'differs in that it:', + ...indent(which), + ]); + }); } - - Subject get debugHtmlText => has((n) => n.debugHtmlText, 'debugHtmlText'); } -extension ZulipContentChecks on Subject { - Subject> get nodes => has((n) => n.nodes, 'nodes'); -} +Iterable? _compareDiagnosticsNodes(DiagnosticsNode actual, DiagnosticsNode expected) { + assert(actual is DiagnosticableTreeNode && expected is DiagnosticableTreeNode); -extension BlockContentNodeListChecks on Subject> { - void equalsNodes(List expected) { - deepEquals(expected.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(checks): report that as API feedback on deepEquals + if (actual.value.runtimeType != expected.value.runtimeType) { + return [ + 'has type ${actual.value.runtimeType}', + 'expected: ${expected.value.runtimeType}', + ]; } -} - -extension BlockInlineContainerNodeChecks on Subject { - Subject> get nodes => has((n) => n.nodes, 'nodes'); -} - -extension ParagraphNodeChecks on Subject { - Subject get wasImplicit => has((n) => n.wasImplicit, 'wasImplicit'); -} - -extension HeadingNodeChecks on Subject { - Subject get level => has((n) => n.level, 'level'); -} - -extension ListNodeChecks on Subject { - Subject get style => has((n) => n.style, 'style'); - Subject>> get items => has((n) => n.items, 'items'); -} -extension QuotationNodeChecks on Subject { - Subject> get nodes => has((n) => n.nodes, 'nodes'); -} + final actualProperties = actual.getProperties(); + final expectedProperties = expected.getProperties(); + assert(actualProperties.length == expectedProperties.length); + for (int i = 0; i < actualProperties.length; i++) { + assert(actualProperties[i].name == expectedProperties[i].name); + if (actualProperties[i].value != expectedProperties[i].value) { + return [ + 'has ${actualProperties[i].name} that:', + ...indent(prefixFirst('is ', literal(actualProperties[i].value))), + ...indent(prefixFirst('expected: ', literal(expectedProperties[i].value))) + ]; + } + } -extension InlineContentNodeListChecks on Subject> { - void equalsNodes(List expected) { - deepEquals(expected.map( - (e) => it()..isA().equalsNode(e))); + final actualChildren = actual.getChildren(); + final expectedChildren = expected.getChildren(); + if (actualChildren.length != expectedChildren.length) { + return [ + 'has ${actualChildren.length} children', + 'expected: ${expectedChildren.length} children', + ]; + } + for (int i = 0; i < actualChildren.length; i++) { + final failure = _compareDiagnosticsNodes(actualChildren[i], expectedChildren[i]); + if (failure != null) { + final diagnosticable = actualChildren[i].value as Diagnosticable; + return [ + 'has child $i (${diagnosticable.toStringShort()}) that:', + ...indent(failure), + ]; + } } -} -extension InlineContainerNodeChecks on Subject { - Subject> get nodes => has((n) => n.nodes, 'nodes'); + return null; }