Skip to content

test: Prevent mutating shared example users or other data #1714

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

gnprice
Copy link
Member

@gnprice gnprice commented Jul 18, 2025

This follows up on #1713 to prevent re-introducing any state-leak bugs of the same class as #1712.

The first two commits fix a couple of existing such bugs, beyond the one fixed in #1713, which just haven't happened to break any tests for us yet. Then we introduce an _ImmutableUser type to use for eg.selfUser and friends, to prevent any other mutation of those shared example users; then close off the remaining few places in example_data.dart which could in principle be affected by such a bug.

427e541 action_sheet test: Avoid mutating shared example users

This is the same sort of state leak that caused #1712; this instance
of it just hasn't happened to break any tests for us yet.

We'll soon arrange things so that this sort of state-leaking mutation
causes an immediate error. This is one of the three total places
where it turns out we had such mutations, including the one we just
fixed in a04b44e (#1713).

The second of these tests ("no error if recipient was deactivated …")
wasn't actually mutating the shared example user eg.otherUser,
because secretly setupToMessageActionSheet makes a new User object
with the same user ID and puts that in the store. Still, it looked
like it was; best to do something that clearly looks correct instead.

The first of these tests was indeed mutating eg.selfUser, just as it
looks like it's doing.

d1eaacb recent-dms test: Avoid mutating eg.selfUser

Like in the parent commit fixing some action-sheet tests, this is a
state leak that just hasn't happened to break any tests for us yet.

In this case, the fix can also simplify the interface that this
prep helper setupPage has with its callers: instead of a specific
feature for setting fullName on the self-user, the function can
take an optional User object for the self-user. Then the individual
test case can do whatever it likes to set up that User object.

19439b3 test: Prevent mutation of shared example users

This ensures we don't re-introduce the sort of state leak that
caused #1712. In three recent commits we fixed every existing
example of this sort of state leak; only three test files had them.

I also looked through all the other top-level fields in this file
example_data.dart. Most are immutable atoms like ints and strings.
There are a handful of remaining points of mutability which could in
principle allow this sort of state leak; so I'll tighten those up too
in the next couple of commits.

d8d19c8 test [nfc]: Make all example-data globals final

It's unlikely we would make the mistake of accidentally mutating these
bindings; but best to rule it out.

edf1eee test: Prevent mutating shared example ServerEmojiData values

In principle these are subject to the same sort of state leak we've
run into with the shared example User objects, and fixed in recent
commits: these are shared global objects, which don't get discarded or
reset by testBinding.reset, and until this commit they were mutable.

The probability that we'd actually end up with such a state leak was
low: ServerEmojiData values never normally get mutated in the app's
data structures (unlike User values), plus there'll probably only ever
be a small number of tests that would have a reason to use these.

But after the preceding couple of commits (notably the one introducing
_ImmutableUser), these represent the last remaining mutable data in
this file's top-level fields. So let's eliminate that too, and get
to 100% in eliminating the possibility of the #1712 class of bug.

gnprice added 5 commits July 17, 2025 18:26
This is the same sort of state leak that caused zulip#1712; this instance
of it just hasn't happened to break any tests for us yet.

We'll soon arrange things so that this sort of state-leaking mutation
causes an immediate error.  This is one of the three total places
where it turns out we had such mutations, including the one we just
fixed in a04b44e (zulip#1713).

The second of these tests ("no error if recipient was deactivated …")
wasn't actually mutating the shared example user `eg.otherUser`,
because secretly `setupToMessageActionSheet` makes a new User object
with the same user ID and puts that in the store.  Still, it *looked*
like it was; best to do something that clearly looks correct instead.

The first of these tests was indeed mutating `eg.selfUser`, just as it
looks like it's doing.
Like in the parent commit fixing some action-sheet tests, this is a
state leak that just hasn't happened to break any tests for us yet.

In this case, the fix can also simplify the interface that this
prep helper `setupPage` has with its callers: instead of a specific
feature for setting `fullName` on the self-user, the function can
take an optional User object for the self-user.  Then the individual
test case can do whatever it likes to set up that User object.
This ensures we don't re-introduce the sort of state leak that
caused zulip#1712.  In three recent commits we fixed every existing
example of this sort of state leak; only three test files had them.

I also looked through all the other top-level fields in this file
`example_data.dart`.  Most are immutable atoms like ints and strings.
There are a handful of remaining points of mutability which could in
principle allow this sort of state leak; so I'll tighten those up too
in the next couple of commits.
It's unlikely we would make the mistake of accidentally mutating these
bindings; but best to rule it out.
In principle these are subject to the same sort of state leak we've
run into with the shared example User objects, and fixed in recent
commits: these are shared global objects, which don't get discarded or
reset by `testBinding.reset`, and until this commit they were mutable.

The probability that we'd actually end up with such a state leak was
low: ServerEmojiData values never normally get mutated in the app's
data structures (unlike User values), plus there'll probably only ever
be a small number of tests that would have a reason to use these.

But after the preceding couple of commits (notably the one introducing
_ImmutableUser), these represent the last remaining mutable data in
this file's top-level fields.  So let's eliminate that too, and get
to 100% in eliminating the possibility of the zulip#1712 class of bug.
@gnprice gnprice requested a review from chrisbobbe July 18, 2025 03:33
@gnprice gnprice added the maintainer review PR ready for review by Zulip maintainers label Jul 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintainer review PR ready for review by Zulip maintainers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant