realm reducer: Start updating createWebPublicStreamPolicy from event

[chrisbobbe]

And sync realm data in /register and the event with the doc at FL 130, including zulip/zulip#22089.

I started going in a direction with the realm-reducer tests that ended up adding too much complexity to be worth it, I think. I've left in two commits in that direction, just in case they're interesting. But I've put them at the tip of the branch, so they're easy to cut out, leaving behind a more reasonable approach.

[gnprice]

Thanks! Comments below.

[gnprice]

Huh, I wonder how this one got left out before.

[gnprice]

I agree, this seems like more complexity than is worth it (given that what's being tested here doesn't really have any tricky interactions.)

[gnprice]

It turns out that this bit of test code doesn't actually do anything! These tests don't run.

The same is true of the existing twenty_four_hour_time tests above.

You can witness this by running Jest directly, and narrowing it down to just the one file so that (by default) it gives more details:

$ npx jest realmReducer --selectProjects ios
Running one project: ios
 PASS   ios  src/realm/__tests__/realmReducer-test.js
  realmReducer
    REGISTER_COMPLETE
      ✓ updates as appropriate on a boring but representative REGISTER_COMPLETE (2 ms)
    ACCOUNT_SWITCH
      ✓ resets state
    EVENT_UPDATE_DISPLAY_SETTINGS
      ✓ change the display settings (1 ms)
    EVENT_REALM_EMOJI_UPDATE
      ✓ update state to new realm_emoji (1 ms)
    EVENT_REALM_FILTERS
      ✓ update state to new realm_filter

Test Suites: 1 passed, 1 total
Tests:       5 passed, 5 total
Snapshots:   0 total
Time:        3.239 s, estimated 4 s
Ran all test suites matching /realmReducer/i.

No mention of these test cases.

The issue is that there's no test call: this describe calls check, and check has an expect but no test around it.

Two reasonable solutions would be:

• Switch this inmost describe to a test: so test('name / name', ….
• Have check wrap its body in test. (Including the logic to construct the initial and expected states -- best to push work inside the test callbacks where possible, because that way Jest can get more quickly through all the outer logic that defines what test cases exist, and have its parallelization magic fully apply to that work.)

[gnprice]

You can witness this by running Jest directly, and narrowing it down to just the one file so that (by default) it gives more details:

BTW this view also displays something I'd want a fix for if we pursued the more complex approach: the individual test cases get extremely long names. Here's a single one of them:

        ✓ 

initial state: {
  name: 'foo',
  description: 'foo',
  nonActiveUsers: [ [length]: 0 ],
  filters: [ [length]: 0 ],
  emoji: {},
  videoChatProvider: null,
  mandatoryTopics: false,
  messageContentDeleteLimitSeconds: null,
  messageContentEditLimitSeconds: 1,
  pushNotificationsEnabled: true,
  webPublicStreamsEnabled: false,
  createWebPublicStreamPolicy: 7,
  enableSpectatorAccess: false,
  canCreateStreams: true,
  isAdmin: false,
  isOwner: false,
  isModerator: false,
  user_id: undefined,
  email: undefined,
  crossRealmBots: [ [length]: 0 ],
  twentyFourHourTime: false
}

 event data: {
  name: 'foo',
  description: 'bar',
  enable_spectator_access: false,
  create_web_public_stream_policy: 6
}

 expected state: {
  name: 'foo',
  description: 'bar',
  nonActiveUsers: [ [length]: 0 ],
  filters: [ [length]: 0 ],
  emoji: {},
  videoChatProvider: null,
  mandatoryTopics: false,
  messageContentDeleteLimitSeconds: null,
  messageContentEditLimitSeconds: 1,
  pushNotificationsEnabled: true,
  webPublicStreamsEnabled: false,
  createWebPublicStreamPolicy: 6,
  enableSpectatorAccess: false,
  canCreateStreams: true,
  isAdmin: false,
  isOwner: false,
  isModerator: false,
  user_id: undefined,
  email: undefined,
  crossRealmBots: [ [length]: 0 ],
  twentyFourHourTime: false
}

Then there are about 700 of those, so together they spew a huge amount of output to the terminal.

[chrisbobbe]

It turns out that this bit of test code doesn't actually do anything! These tests don't run.

Oh wow! Thanks for catching that! 🤯

BTW this view also displays something I'd want a fix for if we pursued the more complex approach: the individual test cases get extremely long names. Here's a single one of them:

…

Then there are about 700 of those, so together they spew a huge amount of output to the terminal.

Yeah, I noticed this problem but decided to check in before spending time trying to solve it (which would mean adding even more complexity).

[chrisbobbe]

• Have check wrap its body in test. (Including the logic to construct the initial and expected states -- best to push work inside the test callbacks where possible, because that way Jest can get more quickly through all the outer logic that defines what test cases exist, and have its parallelization magic fully apply to that work.)

I'll use this approach for my next revision; I like how it lets me give test names more granularly.

[chrisbobbe]

Thanks for the review! Revision pushed.

[gnprice]

Thanks! Looks good modulo a small comment below; then please merge at will.

[gnprice]

This doesn't have an effect -- the rule is only checking that a test callback contains an expect call. Here, the check calls come outside test, not inside, and the test callbacks already contain expect. So we can just leave this line out.

The rule we'd want which would have caught this issue would be that a describe callback contains a test call. I don't see such a rule in the plugin, unfortunately:
https://github.com/jest-community/eslint-plugin-jest#rules

There is no-standalone-expect, sort of a converse, which would also have caught this… but in our eslintrc we disabled it because it's buggy:

  # The docs for this rule [1] say that placing an `expect` call
  # outside a `test` or `it` block (e.g., in a `beforeEach` or
  # `afterEach`) means that the assertion isn't executed. Empirically,
  # this seems just wrong [2], and there are a few places where we
  # want to call `expect` in a `beforeEach` or `afterEach` to assert
  # that multiple tests in the same file aren't interfering with each
  # other by leaving bits of state lying around.
  #
  # [1] https://github.com/jest-community/eslint-plugin-jest/blob/main/docs/rules/no-standalone-expect.md
  # [2] https://github.com/zulip/zulip-mobile/pull/4235#discussion_r489984717
  jest/no-standalone-expect: off

(Also from its docs it sounds like the rule wouldn't actually have caught this anyway? "It is viable, however, to have an expect in a helper function that is called from within a test or it block so expect statements in a function will not trigger this rule.")

[chrisbobbe]

Ah, right, thanks for the investigation!

[chrisbobbe]

Thanks for the review! Merged, with that tweak.