Skip to content
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

accountsReducer: Remove unhelpful test. #4198

Merged
merged 1 commit into from
Jul 22, 2020

Conversation

chrisbobbe
Copy link
Contributor

With Flow v0.113.0, which we'll get in the RN v0.61 -> v0.62 upgrade
(#3782), red flags are raised about this test. Strangely, the test
was allowed to expect the zulipVersion on an Account to be
undefined. In fact, the type for it is ZulipVersion | null.

The test is based on the false premise that a REALM_INIT action
might not have a zulipVersion property. RealmInitAction states
that it will always be there, as a ZulipVersion instance. So, delete
the test. If we ever find that there's a legitimate case where it is
missing, we'll fix the RealmInitAction type accordingly, and a new
test can proceed from there.

@chrisbobbe chrisbobbe requested a review from gnprice July 20, 2020 17:28
@chrisbobbe chrisbobbe changed the title accountsReducer: Remove confusing test. accountsReducer: Remove unhelpful test. Jul 20, 2020
@chrisbobbe chrisbobbe force-pushed the pr-rm-confusing-test branch 2 times, most recently from 026717e to 013eb9a Compare July 20, 2020 18:04
With Flow v0.113.0, which we'll get in the RN v0.61 -> v0.62 upgrade
(zulip#3782), red flags are raised about this test. Strangely [1], the
test was allowed to expect the `zulipVersion` on an Account to be
undefined. In fact, the type for it is `ZulipVersion | null`.

The test is based on the false premise that a REALM_INIT action
might not have a `zulipVersion` property. `RealmInitAction` states
that it will always be there, as a ZulipVersion instance. So, delete
the test. If we ever find that there's a legitimate case where it is
missing, we'll fix the RealmInitAction type accordingly, and a new
test can proceed from there.

[1] I think the culprit is very likely facebook/flow#6715, which Ray
    reported as being fixed in Flow v0.111.1, at
    zulip#4046 (comment)
@gnprice
Copy link
Member

gnprice commented Jul 22, 2020

Merged, thanks!

@gnprice gnprice merged commit 4b5aaba into zulip:master Jul 22, 2020
@chrisbobbe chrisbobbe deleted the pr-rm-confusing-test branch November 6, 2020 03:20
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.

None yet

2 participants