Extend exampleData to further clean up messageActionSheet tests #3946

gnprice · 2020-03-03T01:30:48Z

Thanks @agrawal-d for upgrading the message action sheet tests in #3824 / 4670210 to be well-typed! And thanks @ray-kraesig for reviewing and merging that PR.

After reading that commit, I saw some ways that that test file could be further improved to reduce boilerplate and also to make the test data more typical and more fully coherent (e.g., stream name and ID properties agreeing with each other.) Some of those involve extending exampleData with new features. This PR is the result.

(@agrawal-d , I can't seem to add you as a requested reviewer -- perhaps you're not in the GitHub org? But please do take a look. 🙂 )

agrawal-d · 2020-03-03T15:47:15Z

I have a few examinations this week, I'll take a look once they are over.

agrawal-d · 2020-03-04T14:06:49Z

src/message/__tests__/messageActionSheet-test.js

@@ -1,5 +1,6 @@
 // @flow strict-local
 import deepFreeze from 'deep-freeze';
+


Oh! I didn't know about this!

I have one question - In several files, there is a newline after // @flow strict-local, in some files there isn't. I couldn't decipher any pattern. What is the correct practice?

In several files, there is a newline after // @flow strict-local, in some files there isn't. I couldn't decipher any pattern. What is the correct practice?

Reasonable question!

I think we don't really have a set style. :-) I can't think of a particular preference I really have, either.

I did just now go look for examples of how I've done it myself, when adding a new file or adding type-checking to a file. I used the command git lsp --author greg -S '@flow strict-local' (i.e. git log --stat -p --author greg -S '@flow strict-local' but I have an alias for log --stat -p), and used "the secret to git log -p" to conveniently browse the results.

The result is... I do both. 😄

agrawal-d · 2020-03-04T14:31:54Z

src/__tests__/exampleData.js

+    pin_to_top: false,
+    audible_notifications: false,
+    desktop_notifications: false,
+    email_address: '??? make this value representative before using in a test :)',


I had a look at zulip/zulip's zerver/lib/email_mirror_helpers.py:32, here's how the stream email is generated:

def encode_email_address_helper(name: str, email_token: str, show_sender: bool=False) -> str: # Some deployments may not use the email gateway if settings.EMAIL_GATEWAY_PATTERN == '': return '' # Given the fact that we have almost no restrictions on stream names and # that what characters are allowed in e-mail addresses is complicated and # dependent on context in the address, we opt for a simple scheme: # 1. Replace all substrings of non-alphanumeric characters with a single hyphen. # 2. Use Django's slugify to convert the resulting name to ascii. # 3. If the resulting name is shorter than the name we got in step 1, # it means some letters can't be reasonably turned to ascii and have to be dropped, # which would mangle the name, so we just skip the name part of the address. name = re.sub(r"\W+", '-', name) slug_name = slugify(name) encoded_name = slug_name if len(slug_name) == len(name) else '' # If encoded_name ends up empty, we just skip this part of the address: if encoded_name: encoded_token = "%s.%s" % (encoded_name, email_token) else: encoded_token = email_token if show_sender: encoded_token += ".show-sender" return settings.EMAIL_GATEWAY_PATTERN % (encoded_token,)

I'm not at all familiar with Python, but it looks like we use a combination of the stream name in ASCII and some server configs to generate the stream name. Perhaps, this might help us in using a representative value here.

Yeah -- the easiest way to get a representative value is probably to look at some empirical data, e.g. in the console when debugging the app.

But also it's not a priority to fill in this particular property's value, because we don't have any tests that care about it. I suspect it's something we actually completely don't care about in the app 😉, and so ideally would just drop and not even store in Redux.

agrawal-d · 2020-03-04T14:52:11Z

src/__tests__/exampleData.js

+    audible_notifications: false,
+    desktop_notifications: false,
+    email_address: '??? make this value representative before using in a test :)',
+    is_old_stream: true,


I couldn't find the meaning of is_old_stream in the codebase - can you please tell me? Also, it doesn't appear to be used anywhere at all ( I am probably wrong but a search in VS Code for is_old_stream gives only declarations, no uses. )

From a scan of the server code, it seems to mean "this stream is old enough that the stream_weekly_traffic number is meaningful", and concretely "this stream is at least 7 days old".

It's not a great name 😉

Won't matter to us unless/until we start doing something with stream_weekly_traffic too.

agrawal-d

I took a look at this as you suggested - I learnt a lot. Thanks! The commit messages were very helpful, and I can definitely see how these changes make the tests and exampleData cleaner.

(Please don't mind the 'approved' tag - I accidentally clicked it, these are just general comments.)

agrawal-d · 2020-03-04T14:58:02Z

src/message/__tests__/messageActionSheet-test.js

@@ -157,7 +144,7 @@ describe('constructHeaderActionButtons', () => {

    const subscriptions = deepFreeze([
      {
-        ...baseBackgroundData.subscriptions[0],
+        ...eg.subscription,


If in_home_view is already true in makeSubscription then should we still provide it here - Did you add it for readability?

Yeah -- specifically, I left it explicit here because it's part of what this test is about.

If it were omitted, then looking at this test you wouldn't be able to tell whether the test actually tests the case it's meant to test unless you knew that eg.subscription definitely has in_home_view: true. That is the logical default, and so you could perhaps figure that it ought to be the default... but even to know it's the logical default, you have to know, and think about, some context about what it means.

Whereas when it's explicitly here, then you can look at this test code very locally, and think through it at just a very straightforward step-by-step level, and see that it does exactly what it says it does.

agrawal-d · 2020-03-04T15:02:14Z

src/message/__tests__/messageActionSheet-test.js

@@ -42,9 +36,11 @@ describe('constructActionButtons', () => {

  test('show unstar message option if message is starred', () => {
    const message = eg.streamMessage({ id: 1 });
+    // eslint-disable-next-line no-useless-computed-key
+    const flags = { ...baseBackgroundData.flags, starred: { [1]: true, [2]: true } };


This is a nice trick! :-)

agrawal-d · 2020-03-04T15:19:09Z

src/message/__tests__/messageActionSheet-test.js

-        in_home_view: true,
-      },
-    ]);
+    const subscriptions = [{ ...eg.subscription, in_home_view: true }];


Same as above - Did you add in_home_view: true for readability?

(same answer)

rk-for-zulip

Thanks! One note and one nit; ✔️ otherwise.

rk-for-zulip · 2020-03-10T21:43:16Z

src/api/modelTypes.js

@@ -440,7 +440,7 @@ export type Submessage = $ReadOnly<{|
 * See also `MessagesState` for discussion of how we fetch and store message
 * data.
 */
-export type Message = $ReadOnly<{
+export type Message = $ReadOnly<{|


Some thoughts, taken from a draft branch on my local machine:

@@ -493,4 +493,13 @@ export type Message = $ReadOnly<{ timestamp: number, type: 'stream' | 'private', + + // This type is not exact. The server may have sent us arbitrary data, so it's + // not safe to {...spread} objects of this type. + // + // TODO: destructure and validate message data received from the Zulip server, + // transforming it into a normalized, exact `Message` type strictly for app- + // internal use. (See docs/architecture/crunchy-shell.md.) + + ... }>;

Really, none of the server-sent data types should be exact – future revisions of the server will almost certainly add new fields to some of them, and we shouldn't make promises that we know won't be kept. 😕

Really, none of the server-sent data types should be exact – future revisions of the server will almost certainly add new fields to some of them, and we shouldn't make promises that we know won't be kept. 😕

When those future revisions arrive, though, we'll try to update the types accordingly, right? Having exact types with temporary out-of-date states feels better, to me, than making everything inexact. We have server and client capabilities to handle migrations gracefully, and we (aim to) keep careful notes on fields about when they were introduced / removed / changed.

+ // TODO: destructure and validate message data received from the Zulip server, + // transforming it into a normalized, exact `Message` type strictly for app- + // internal use. (See docs/architecture/crunchy-shell.md.)

This sounds right as an eventual goal, though.

rk-for-zulip · 2020-03-10T21:48:22Z

src/message/__tests__/messageActionSheet-test.js

@@ -30,9 +22,11 @@ describe('constructActionButtons', () => {

  test('show star message option if message is not starred', () => {
    const message = eg.streamMessage({ id: 3 });
+    // eslint-disable-next-line no-useless-computed-key


I think it's worth linking to facebook/flow#380 around here somewhere.

Also demonstrate using them, to reduce boilerplate in the newly-well-typed messageActionSheet tests. Several values change, generally to a more representative value or a default value. One notable one is `in_home_view`: this means "not muted", and so it's an important fact that its default is true. [chris: cherry-picked from zulip#3946.]

Also demonstrate using them, to reduce boilerplate in the newly-well-typed messageActionSheet tests. Several values change, generally to a more representative value or a default value. One notable one is `in_home_view`: this means "not muted", and so it's an important fact that its default is true. [cherry-picked from zulip#3946.]

This is a small formatting nit, maintaining a pattern we generally observe throughout the codebase.

This looks like interesting detail that should matter somehow, but in fact it doesn't. Make the data totally boring instead, to help the reader focus on the test details that do matter.

This allows us to skip reciting this boring stuff in a place where the code doesn't actually care about it.

Also demonstrate using them, to reduce boilerplate in the newly-well-typed messageActionSheet tests. Several values change, generally to a more representative value or a default value. One notable one is `in_home_view`: this means "not muted", and so it's an important fact that its default is true.

chrisbobbe · 2020-07-20T20:41:07Z

@gnprice I'm taking a look at this now, and will likely merge soon unless you know of obstacles and haven't written them down yet. 🙂

The way this file had worked for a while was that this flags object, with these particular arbitrary message IDs, was used globally in the file and then these two particular tests used message IDs that just happened to have specific relationships to those here. That's no good. :-) It makes these tests harder to read, by obscuring their relationship to the data they're actually testing; and it makes all the other tests harder to read, by raising the possibility that they might somehow depend on these oddly-specific `flags` values. Instead, push this data down into the specific tests that use it. Much clearer. Also demonstrate how to avoid the Flow error: write the key as an actual number. The trick is just to tell ESLint to go away when it tries to rewrite the key as a (numeric-looking) string. [1] [1] facebook/flow#380 [chris: link to Flow issue in code and commit message]

Also demonstrate using the messages' IDs, replacing arbitrary IDs like 1, 2, and 3 in the messageActionSheet tests.

And demonstrate using it, cutting some boilerplate in a test. This also lets us make the emoji code and name both real, and in agreement with each other.

This is preferable to passing properties like `display_recipient` or `stream_id`, because it encapsulates the work of ensuring those are kept aligned with each other. [chris: tweak to respond to a Flow warning that probably came from the recent React Native upgrade]

It's often convenient in a test to have a message in a given stream, a subscription to the same stream, etc. Our baseline example objects have the natural relationships of this kind, and deliberately so because it is so often convenient and because it's easy to make new unrelated objects when that's desired. Document those relationships, to distinguish them from any accidental arbitrary facts about these example objects, so that we can cheerfully rely on them in tests.

The Subscription object has both a stream name and a stream ID, and so to keep the test data well-formed these should be kept aligned in referring to the same stream. Similarly the Message object. Right now to make the test pass it's enough to set the stream name, because we don't use the stream ID; but that's a case of zulip#3918 and should change, after which the mismatch will be a live cause of test failures. We could do this by passing a stream object to `eg.makeSubscription` and to `eg.streamMessage`. But the most natural one to use is `eg.stream`; and the example data already refers to `eg.stream` by default in all the natural places, among them `eg.streamMessage()` and `eg.subscription`, so simply use that.

This makes a little less background noise in reading these tests from the aspects that are boring, helping the parts that are distinctive to each test stand out. By making the file about 25% shorter, it also helps look at several tests at once so as to see what they're doing in common and how they differ.

chrisbobbe · 2020-07-20T21:46:15Z

Merged, thanks, @gnprice! I made two small tweaks:

I linked to facebook/flow#380 as Ray suggested at #3946 (comment).

I got a Flow warning in

7d9e7f3 exampleData: In eg.streamMessage, take an optional stream.

that probably wasn't showing up when this PR was first composed; we've upgraded Flow since then. I addressed its complaint.

gnprice · 2020-07-21T02:26:32Z

Thanks @chrisbobbe !

That Flow warning is pretty odd. I spent a few minutes investigating and wound up with a diagnosis and a cleaner workaround -- so I pushed that just now as a small tweak, d141be7.

gnprice requested a review from rk-for-zulip March 3, 2020 01:30

agrawal-d reviewed Mar 4, 2020

View reviewed changes

agrawal-d approved these changes Mar 4, 2020

View reviewed changes

rk-for-zulip reviewed Mar 10, 2020

View reviewed changes

chrisbobbe mentioned this pull request Jul 10, 2020

[REVIEWABLE] Stop using legacy Context API for styles #4046

Closed

chrisbobbe mentioned this pull request Jul 10, 2020

Make state.narrows an Immutable.Map. #4187

Merged

gnprice force-pushed the pr-exampledata branch from 7396df5 to ff3770a Compare July 13, 2020 21:21

chrisbobbe mentioned this pull request Jul 20, 2020

Stop using legacy Context API for styles (continued) #4199

Merged

gnprice added 4 commits July 20, 2020 12:29

messageActionSheet tests [nfc]: Separate third- and first-party imports.

ba0f97d

This is a small formatting nit, maintaining a pattern we generally observe throughout the codebase.

messageActionSheet tests: Drop a bit of unused entropy in data.

f444bae

This looks like interesting detail that should matter somehow, but in fact it doesn't. Make the data totally boring instead, to help the reader focus on the test details that do matter.

messageActionSheet tests [nfc]: Use exampleData a bit more.

99d2ce5

This allows us to skip reciting this boring stuff in a place where the code doesn't actually care about it.

gnprice added 7 commits July 20, 2020 13:56

exampleData: Use random message IDs in pmMessage and streamMessage.

affcb9c

Also demonstrate using the messages' IDs, replacing arbitrary IDs like 1, 2, and 3 in the messageActionSheet tests.

exampleData: Add eg.unicodeEmojiReaction.

9522593

And demonstrate using it, cutting some boilerplate in a test. This also lets us make the emoji code and name both real, and in agreement with each other.

chrisbobbe force-pushed the pr-exampledata branch from ff3770a to 87f8c8e Compare July 20, 2020 21:45

chrisbobbe merged commit 87f8c8e into zulip:master Jul 20, 2020

gnprice deleted the pr-exampledata branch March 14, 2022 19:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend exampleData to further clean up messageActionSheet tests #3946

Extend exampleData to further clean up messageActionSheet tests #3946

gnprice commented Mar 3, 2020

agrawal-d commented Mar 3, 2020

agrawal-d Mar 4, 2020

gnprice Mar 10, 2020

agrawal-d Mar 4, 2020

gnprice Mar 10, 2020

agrawal-d Mar 4, 2020

gnprice Mar 10, 2020

agrawal-d left a comment •

edited

Loading

agrawal-d Mar 4, 2020 •

edited

Loading

gnprice Mar 10, 2020

agrawal-d Mar 4, 2020

agrawal-d Mar 4, 2020 •

edited

Loading

gnprice Mar 10, 2020

rk-for-zulip left a comment

rk-for-zulip Mar 10, 2020

chrisbobbe Jul 20, 2020 •

edited

Loading

chrisbobbe Jul 20, 2020

rk-for-zulip Mar 10, 2020

chrisbobbe commented Jul 20, 2020

chrisbobbe commented Jul 20, 2020

gnprice commented Jul 21, 2020

		@@ -1,5 +1,6 @@
		// @flow strict-local
		import deepFreeze from 'deep-freeze';

Extend exampleData to further clean up messageActionSheet tests #3946

Extend exampleData to further clean up messageActionSheet tests #3946

Conversation

gnprice commented Mar 3, 2020

agrawal-d commented Mar 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawal-d left a comment • edited Loading

Choose a reason for hiding this comment

agrawal-d Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agrawal-d Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rk-for-zulip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe Jul 20, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisbobbe commented Jul 20, 2020

chrisbobbe commented Jul 20, 2020

gnprice commented Jul 21, 2020

agrawal-d left a comment •

edited

Loading

agrawal-d Mar 4, 2020 •

edited

Loading

agrawal-d Mar 4, 2020 •

edited

Loading

chrisbobbe Jul 20, 2020 •

edited

Loading