search

dart-lang / markdown

A Dart markdown library
https://pub.dev/packages/markdown
BSD 3-Clause "New" or "Revised" License
443 stars 201 forks source link

create a workflow to smoke test package:flutter_markdown #481

Closed devoncarew closed 1 year ago

devoncarew commented 1 year ago

~creating as a draft PR while I iron out issues~

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 3391434200

Totals Coverage Status
Change from base Build 3363093491: 0.0%
Covered Lines: 1198
Relevant Lines: 1267
💛 - Coveralls
devoncarew commented 1 year ago

Success!

It's failing, but that's expected (tracked in #480). Here's the issue in-lined:

══╡ EXCEPTION CAUGHT BY WIDGETS LIBRARY ╞═══════════════════════════════════════════════════════════
The following UnsupportedError was thrown building Directionality(textDirection: ltr):
Unsupported operation: Cannot add to an unmodifiable list

The relevant error-causing widget was:
  Directionality
  Directionality:file:///home/runner/work/markdown/markdown/flutter_packages/packages/flutter_markdown/test/utils.dart:164:10

When the exception was thrown, this was the stack:
#0      UnmodifiableListMixin.add (dart:_internal/list.dart:114:5)
#1      WikilinkSyntax.onMatch (file:///home/runner/work/markdown/markdown/flutter_packages/packages/flutter_markdown/test/custom_syntax_test.dart:165:18)
#2      InlineSyntax.tryMatch (package:markdown/src/inline_syntaxes/inline_syntax.dart:49:9)
#3      InlineParser.parse.<anonymous closure> (package:markdown/src/inline_parser.dart:121:43)
#4      ListMixin.any (dart:collection/list.dart:[149](https://github.com/dart-lang/markdown/actions/runs/3366377531/jobs/5582801560#step:9:150):15)
#5      InlineParser.parse (package:markdown/src/inline_parser.dart:121:20)
#6      Document.parseInline (package:markdown/src/document.dart:76:67)
#7      Document._parseInlineContent (package:markdown/src/document.dart:82:29)
#8      Document._parseInlineContent (package:markdown/src/document.dart:87:9)
#9      Document.parseLines (package:markdown/src/document.dart:71:5)
#10     _MarkdownWidgetState._parseMarkdown (package:flutter_markdown/src/widget.dart:320:45)
#11     _MarkdownWidgetState.didChangeDependencies (package:flutter_markdown/src/widget.dart:284:5)
#12     StatefulElement._firstBuild (package:flutter/src/widgets/framework.dart:5107:11)
#13     ComponentElement.mount (package:flutter/src/widgets/framework.dart:4932:5)
...     Normal element mounting (7 frames)
devoncarew commented 1 year ago

I believe the actual https://github.com/flutter/packages repo may be tested against flutter stable and flutter master? We're testing against flutter beta; the skew may become relevant at some point.

kevmoo commented 1 year ago

@devoncarew – so yay it's failing. Now what?

devoncarew commented 1 year ago

@devoncarew – so yay it's failing. Now what?

Sam has a fix upstream - https://github.com/flutter/packages/pull/2758. We can land this after.

devoncarew commented 1 year ago

I'm seeing:

table_test.dart: Table should work with alignments                                                  
══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test:
Expected: TextAlign:<TextAlign.right>
  Actual: TextAlign:<TextAlign.left>

When the exception was thrown, this was the stack:
#4      defineTests.<anonymous closure>.<anonymous closure> (file:///home/runner/work/markdown/markdown/flutter_packages/packages/flutter_markdown/test/table_test.dart:61:9)

and potentially two copies of:

flutter_markdown/test/table_test.dart: Table GFM Examples remainder of table cells may vary, excess cells are ignored  
══╡ EXCEPTION CAUGHT BY WIDGETS LIBRARY ╞═══════════════════════════════════════════════════════════
The following UnsupportedError was thrown building Directionality(textDirection: ltr):
Unsupported operation: Cannot add to an unmodifiable list

The relevant error-causing widget was:
  Directionality
  Directionality:file:///home/runner/work/markdown/markdown/flutter_packages/packages/flutter_markdown/test/utils.dart:164:10

When the exception was thrown, this was the stack:
#0      UnmodifiableListMixin.add (dart:_internal/list.dart:114:5)
#1      MarkdownBuilder.visitElementBefore (package:flutter_markdown/src/builder.dart:263:27)
...
srawlins commented 1 year ago

Perhaps there are remaining new incompatibilities with the two packages.

devoncarew commented 1 year ago

Perhaps there are remaining new incompatibilities with the two packages.

It looks like that's the case. I could adjust this new job to be continue-on-error: true, and track the new failures in an issue (we'd revert the allow failure once the new issues are addressed)?

devoncarew commented 1 year ago

Hmm, it looks like continue-on-error: true doesn't do what I was hoping it would do (have the bot pass even on failures).

kevmoo commented 1 year ago

@devoncarew – I'd say go ahead and land the anyOf thing we chatted about in flutter_markdown. Validate locally, etc.

devoncarew commented 1 year ago

@chenzhiguang - some of the issues may have been introduced by https://github.com/dart-lang/markdown/pull/472. I don't know if the parts of that PR should be reverted, or, if we should address the issues downstream in package:flutter_markdown itself (if it's relying on either implementation details or previous bugs from markdown).

In either case, it would be nice to get the repos to a point where we can land this PR and then incrementally work from there.

kevmoo commented 1 year ago

@devoncarew I did a bisect. It's https://github.com/dart-lang/markdown/pull/472 that started the breaks.

chenzhiguang commented 1 year ago

@devoncarew @kevmoo I am working on it know.

kevmoo commented 1 year ago

@devoncarew – well, it looks like we moved from style="text-align: xyz to align=xyz per spec

This seems to be a big reason for the failure

kevmoo commented 1 year ago 
@@ -470,11 +472,11 @@ class MarkdownBuilder implements md.NodeVisitor {
         current.children.add(_buildRichText(const TextSpan(text: '\n')));
       } else if (tag == 'th' || tag == 'td') {
         TextAlign? align;
-        final String? style = element.attributes['style'];
+        final String? style = element.attributes['align'];
         if (style == null) {
           align = tag == 'th' ? styleSheet.tableHeadAlign : TextAlign.left;
         } else {
-          final RegExp regExp = RegExp(r'text-align: (left|center|right)');
+          final RegExp regExp = RegExp(r'(left|center|right)');
           final Match match = regExp.matchAsPrefix(style)!;
           switch (match[1]) {
             case 'left':

this fixes one of the bugs

The other thing: flutter_markdown MUTATES the element tree, but @chenzhiguang makes children in one case const which is another failure...

srawlins commented 1 year ago

Thanks for catching that Kevin; I missed this in the review; we should be sticking to <td style="text-align" rather than td align=". We should revert that part of the PR. I've commented on it.

chenzhiguang commented 1 year ago 
+          final RegExp regExp = RegExp(r'(left|center|right)');
           final Match match = regExp.matchAsPrefix(style)!;
           switch (match[1]) {

Yep, I found it too. I think here switch (style) is enought.

kevmoo commented 1 year ago

Thanks for catching that Kevin; I missed this in the review; we should be sticking to <td style="text-align" rather than td align=". We should revert that part of the PR. I've commented on it.

See my comment on the original PR

"deprecated" is tricky. This syntax will be supported FOR EVER. If there are zero concerns about it working, sticking to spec will save us a lot of sanity in the future IMHO

chenzhiguang commented 1 year ago

Another failure is caused by this change too: https://github.com/dart-lang/markdown/pull/472/files#diff-870f2b2d4efaa2ddfd880649b3f64ca591e1493372b74a00a8dd51c44de239abR53

- children.add(Element.empty('td'));
+ children.add(Element('td', const []));

Element.empty('td') has a Null children, while Element('td', const []) has an empty children.

This change itself has no problem, as the output of an empty td should be <td></td> instead of <td />.

chenzhiguang commented 1 year ago

Do I need to do any change on our side?

kevmoo commented 1 year ago

Do I need to do any change on our side?

@chenzhiguang – I pushed one small change on our side (dropped const [] in favor of []) – other fixes should be in flutter_markdown, I think!

kevmoo commented 1 year ago

@devoncarew @srawlins @chenzhiguang - https://github.com/flutter/packages/pull/2777

kevmoo commented 1 year ago

Haza! Fixed @devoncarew !!