notif: Add more tests for messaging style notif and fix for fetchBitmap #813

rajveermalviya · 2024-07-15T20:01:49Z

Add more tests for Android messaging style notif implementation, listed here: #718 (review)

Khader-1

Thanks @rajveermalviya! Aside from some nits this looks good.
Also, I'd suggest to change the title of the PR to mention the update in the production code as well.

lib/notifications/display.dart

test/notifications/display_test.dart

rajveermalviya · 2024-07-16T09:22:17Z

Thanks for the review @Khader-1, new revision pushed.

Khader-1

Thanks @rajveermalviya for the revision! Just a tiny nit that I didn't catch in the previous review.

test/notifications/display_test.dart

Khader-1

Thanks @rajveermalviya! LGTM. Over to mentor review now.

kenclary

looks good

chrisbobbe · 2024-07-19T22:44:01Z

With this command:

git rebase --exec 'tools/check --diff @~'

(running tools/check --diff @~ on each commit) I noticed there are some test failures in the first commit:

notif: Ensure fetchBitmap succeeds only on HTTP 200 status

After you fix those (and any failures on other commits; I haven't checked), I'll give this a proper review. 🙂

rajveermalviya · 2024-07-28T20:32:34Z

Thanks for the review @chrisbobbe, pushed a new revision, PTAL.

chrisbobbe

Thanks! Small comments below, and I'll mark this for Greg's review.

chrisbobbe · 2024-07-29T22:12:57Z

test/notifications/display_test.dart

+    T Function() callback, {
+    http.Client Function()? httpClientFactory,
+  }) {
+    return http.runWithClient(callback, httpClientFactory ?? () => fakeHttpClient);


For reading the ?? () => fakeHttpClient, I think it could help if fakeHttpClient had a more specific name, like fakeHttpClientGivingSuccess or something.

chrisbobbe · 2024-07-29T22:15:49Z

test/notifications/display_test.dart

        expectedTagComponent: expectedTagComponent);
    })));

+    test('stream message: multiple messages, different topics', () => runWithHttpClient(() => awaitFakeAsync((async) async {


notif test: Add more tests for messaging style notif Add more tests for Android messaging style notif implementation, listed here: #718 (review)

It looks like you meant to link to a specific comment in #718, but there isn't a link to the comment.

gnprice

Thanks @rajveermalviya for writing this, and @Khader-1 @chrisbobbe for the previous reviews!

These tests look great. I agree with Chris's small comments above, and have a few nits below. Then this will be all ready to merge.

gnprice · 2024-07-29T23:08:47Z

test/notifications/display_test.dart

  TestZulipBinding.ensureInitialized();
  final zulipLocalizations = GlobalLocalizations.zulipLocalizations;

+  makeFakeHttpClient({http.Response? response, Exception? exception}) => http_testing.MockClient((request) async {


nit: explicit return type for function declaration, so it doesn't look like a function call

Looks like there is indeed a lint rule for this, always_declare_return_types:
https://dart.dev/tools/linter-rules/always_declare_return_types

I'll see about turning that on, since it keeps coming up.

separate nit: use block body:
https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#consider-using--for-short-functions-and-methods

I'll see about turning that on, since it keeps coming up.

→ #851

gnprice · 2024-07-29T23:12:39Z

test/notifications/display_test.dart

+  });
+  final fakeHttpClient = makeFakeHttpClient(


nit: blank line

Suggested change

});

final fakeHttpClient = makeFakeHttpClient(

});

final fakeHttpClient = makeFakeHttpClient(

rajveermalviya · 2024-07-30T06:52:03Z

Thanks for the review @chrisbobbe, @gnprice. Pushed a new revision, PTAL.

gnprice

Thanks again! Looks great — merging.

gnprice · 2024-07-30T16:30:33Z

test/notifications/display_test.dart

+        expectedTitle: otherUser.fullName,
+        expectedTagComponent: expectedTagComponent);
+
+      otherUser.email = '[email protected]';


Ah good catch — I missed this in my review of the previous revision 🙂

Add more tests for Android messaging style notif implementation, listed here: zulip#718 (review)

…ion`

gnprice mentioned this pull request Jul 15, 2024

notif: Support messaging-style notifications #718

Merged

rajveermalviya force-pushed the pr-tests-messaging-style branch 2 times, most recently from c7d47ce to b59b751 Compare July 15, 2024 21:43

rajveermalviya changed the title ~~notif: messaging style tests~~ notif test: Expand test coverage for messaging style notif Jul 15, 2024

rajveermalviya marked this pull request as ready for review July 15, 2024 21:56

rajveermalviya added the buddy review GSoC buddy review needed. label Jul 15, 2024

rajveermalviya requested a review from Khader-1 July 15, 2024 22:38

Khader-1 reviewed Jul 16, 2024

View reviewed changes

lib/notifications/display.dart Outdated Show resolved Hide resolved

test/notifications/display_test.dart Outdated Show resolved Hide resolved

test/notifications/display_test.dart Outdated Show resolved Hide resolved

rajveermalviya force-pushed the pr-tests-messaging-style branch 2 times, most recently from 66738ac to 09c6cab Compare July 16, 2024 09:19

rajveermalviya changed the title ~~notif test: Expand test coverage for messaging style notif~~ notif: Add more tests for messaging style notif and fix for fetchBitmap Jul 16, 2024

rajveermalviya requested a review from Khader-1 July 16, 2024 09:22

Khader-1 reviewed Jul 16, 2024

View reviewed changes

test/notifications/display_test.dart Outdated Show resolved Hide resolved

rajveermalviya force-pushed the pr-tests-messaging-style branch from 09c6cab to dadbcba Compare July 16, 2024 10:17

rajveermalviya requested a review from Khader-1 July 16, 2024 10:28

Khader-1 approved these changes Jul 16, 2024

View reviewed changes

Khader-1 requested review from kenclary and sumanthvrao July 16, 2024 10:39

Khader-1 added mentor review GSoC mentor review needed. and removed buddy review GSoC buddy review needed. labels Jul 16, 2024

kenclary approved these changes Jul 16, 2024

View reviewed changes

rajveermalviya added maintainer review PR ready for review by Zulip maintainers and removed mentor review GSoC mentor review needed. labels Jul 16, 2024

rajveermalviya requested a review from chrisbobbe July 16, 2024 15:39

rajveermalviya force-pushed the pr-tests-messaging-style branch 2 times, most recently from a59eb50 to 03444bb Compare July 28, 2024 20:24

chrisbobbe reviewed Jul 29, 2024

View reviewed changes

chrisbobbe added integration review Added by maintainers when PR may be ready for integration and removed maintainer review PR ready for review by Zulip maintainers labels Jul 29, 2024

gnprice reviewed Jul 29, 2024

View reviewed changes

gnprice mentioned this pull request Jul 30, 2024

lint: Enable always_declare_return_types; fix existing places they're missing #851

Merged

rajveermalviya force-pushed the pr-tests-messaging-style branch from 03444bb to 3530d94 Compare July 30, 2024 06:50

rajveermalviya requested a review from gnprice July 30, 2024 06:52

rajveermalviya force-pushed the pr-tests-messaging-style branch from 3530d94 to 289cea8 Compare July 30, 2024 06:58

gnprice reviewed Jul 30, 2024

View reviewed changes

rajveermalviya added 3 commits July 30, 2024 09:30

notif: Ensure fetchBitmap succeeds only on HTTP 200 status

98374b5

notif test: Add more tests for messaging style notif

2c2775a

Add more tests for Android messaging style notif implementation, listed here: zulip#718 (review)

notif [nfc]: Move messageStyleMessages asserts out to `checkNotificat…

915b166

…ion`

gnprice force-pushed the pr-tests-messaging-style branch from 289cea8 to 915b166 Compare July 30, 2024 16:31

gnprice merged commit 915b166 into zulip:main Jul 30, 2024

rajveermalviya deleted the pr-tests-messaging-style branch July 30, 2024 16:33

notif: Add more tests for messaging style notif and fix for fetchBitmap #813

notif: Add more tests for messaging style notif and fix for fetchBitmap #813

Uh oh!

Conversation

rajveermalviya commented Jul 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Khader-1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rajveermalviya commented Jul 16, 2024

Uh oh!

Khader-1 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Khader-1 left a comment

Choose a reason for hiding this comment

Uh oh!

kenclary left a comment

Choose a reason for hiding this comment

Uh oh!

chrisbobbe commented Jul 19, 2024

Uh oh!

rajveermalviya commented Jul 28, 2024

Uh oh!

chrisbobbe left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rajveermalviya commented Jul 30, 2024

Uh oh!

gnprice left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rajveermalviya commented Jul 15, 2024 •

edited

Loading