Skip to content

content test: Share example data between parsing tests and widgets tests #511

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

Merged
merged 16 commits into from
Feb 12, 2024

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Feb 9, 2024

The main goal here is to allow us to reuse in the widget tests in test/widgets/content_test.dart the example content blobs that we've written down in the parsing tests in test/model/content_test.dart. Currently we basically have to duplicate the examples, which is OK for short examples and gets more annoying as the example HTML gets longer than a line or so.

A nice followup would be to convert all our existing parsing tests into the new ContentExample form, and then use testContentSmoke to have smoke widget tests for all of them. (We could even add an "all content examples are tested" meta-test for widget tests, analogous to the one this PR adds for parsing tests.) But the conversions in this PR, focusing on the lengthiest HTML examples, are as much as I have time for at the moment. Meanwhile this provides the framework for future tests to be on ContentExample from the start.

Along the way this branch takes care of a few housekeeping items:

  • sorting our dependencies alphabetically, to reduce the conflicts that come from every change adding to the end of the list;
  • configuring IntelliJ (aka Android Studio) to suppress some spurious complaints, notably in the HTML of the content tests;
  • and along the way to those two, cutting a couple of extraneous items left over from the flutter create template.

This was referenced Feb 9, 2024
@chrisbobbe
Copy link
Collaborator

Thanks, LGTM! I've added a small commit to use ContentExample for quotations, in the model tests, and also to add a corresponding widget smoke test. If I've understood the pattern and it looks good, please merge at will. 🙂

Copy link
Member Author

@gnprice gnprice left a comment

Choose a reason for hiding this comment

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

Thanks! Yep, that looks good, with just an indentation nit. I'll fix that and merge.

Comment on lines 125 to 127
'<blockquote>\n<p>words</p>\n</blockquote>', [
QuotationNode([ParagraphNode(links: null, nodes: [TextNode('words')])])
]);
Copy link
Member Author

Choose a reason for hiding this comment

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

nit: this should get just one level of indentation

Suggested change
'<blockquote>\n<p>words</p>\n</blockquote>', [
QuotationNode([ParagraphNode(links: null, nodes: [TextNode('words')])])
]);
'<blockquote>\n<p>words</p>\n</blockquote>', [
QuotationNode([ParagraphNode(links: null, nodes: [TextNode('words')])])
]);

gnprice and others added 16 commits February 12, 2024 11:50
This was left here by the `flutter create` template, but we've
never used it.  If we do want to use it in the future, we can
always add it back.
To date we've been adding new dependencies to the end of the
lists.  One consequence is that adding any dependency always
has a merge/rebase conflict with adding any other (unless one
is a dev dependency and the other isn't), because the lines go
in the same place.

That's kind of annoying, so let's start keeping these in
alphabetical order instead.  This way also makes it a bit
easier to find a given dependency.

Also delete a comment the template left about how to configure
lints; we've been successfully doing that when it comes up.
These `.idea/` directories contain the project configuration and
state of IntelliJ IDEA and its family of IDEs -- in particular
Android Studio, which is what I and Chris use.  There's one for
opening the repo at its root (for working on any of the Dart code)
and another for opening the android/ subfolder (which better
elicits the IDE's support for working on Android-specific code).

Lots of the files here are local state (right down to the size
and location the IDE window last occupied on the screen) that
don't make sense to share.  But mixed in among those is some
project configuration that is best shared; in particular, this
file configuring which "inspections" to run, i.e. what to
highlight in the editor as problems to be fixed.

Start the file off by disabling the spell-check inspection,
because it unavoidably has tons of false positives unless one
maintains a list of all the words it doesn't know.  I've had this
configuration locally since early on; adding to version control
now partly for its own sake, but also as preparation to add
further configuration to this file.
This Flutter app hasn't worked on the web since we started using a
SQLite database, if not sooner.  That's no loss; our actual deployment
targets are (as README.md says) Android and iOS, and even desktop
platforms are supported only for occasional development convenience.

So just cut out the files that make up the Web support, which were
added by `flutter create` from its templates at the beginning and
which we've barely touched since (a total of once, to capitalize
the name "Zulip").
The only HTML we have in this project is the example fragments of
Zulip content HTML (as in Zulip message bodies) in a couple of our
test files.  The HTML there is almost all real output from a live
Zulip server; even apart from false-positive complaints, there's no
point critiquing its style because this client's role is to consume
this HTML, not emit it.  So turn all the HTML inspections off.

This clears out a few dozen complaints the various inspections had had
in test/model/content_test.dart in particular, so that it's clean of
issues, just like the rest of our files.  That makes it easier to
navigate to genuine lint issues when they arise while developing
changes, and easier to spot occurrence highlights in the scrollbar
to see where else in the file some identifier appears that one has
one's cursor on.
@gnprice gnprice merged commit 9fddec2 into zulip:main Feb 12, 2024
@gnprice gnprice deleted the pr-content-test branch February 12, 2024 19:52
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