Skip to content

Comments

[FS-901] Tests for joining subconversation#2974

Merged
mdimjasevic merged 10 commits intomlsfrom
fs-901/tests-for-joining
Jan 16, 2023
Merged

[FS-901] Tests for joining subconversation#2974
mdimjasevic merged 10 commits intomlsfrom
fs-901/tests-for-joining

Conversation

@mdimjasevic
Copy link
Contributor

@mdimjasevic mdimjasevic commented Jan 11, 2023

Tracked by https://wearezeta.atlassian.net/browse/FS-901. Commit 352e063 that implements proposal execution in subconversations was cherry-picked from PR #2969. Either this PR gets merged with the cherry-picked commit or #2969 gets merged first and then this PR gets rebased.

Checklist

  • Add a new entry in an appropriate subdirectory of changelog.d
  • Read and follow the PR guidelines

@mdimjasevic mdimjasevic temporarily deployed to cachix January 11, 2023 15:03 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 11, 2023 15:03 — with GitHub Actions Inactive
@zebot zebot added the ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist label Jan 11, 2023
@mdimjasevic mdimjasevic force-pushed the fs-901/tests-for-joining branch from bee922a to b4a2a65 Compare January 11, 2023 16:14
@mdimjasevic mdimjasevic temporarily deployed to cachix January 11, 2023 16:14 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 11, 2023 16:14 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic changed the base branch from mls to mdimjasevic/subconv-epoch-0-kpref-fix January 11, 2023 16:15
Base automatically changed from mdimjasevic/subconv-epoch-0-kpref-fix to mls January 12, 2023 10:13
@mdimjasevic mdimjasevic force-pushed the fs-901/tests-for-joining branch from b4a2a65 to dfd5ffc Compare January 12, 2023 10:18
@mdimjasevic mdimjasevic temporarily deployed to cachix January 12, 2023 10:18 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 12, 2023 10:18 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic force-pushed the fs-901/tests-for-joining branch from dfd5ffc to 8c1bf38 Compare January 12, 2023 10:59
@mdimjasevic mdimjasevic temporarily deployed to cachix January 12, 2023 10:59 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 12, 2023 10:59 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic force-pushed the fs-901/tests-for-joining branch from 8c1bf38 to dc2d662 Compare January 12, 2023 12:09
@mdimjasevic mdimjasevic temporarily deployed to cachix January 12, 2023 12:09 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 12, 2023 12:09 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic marked this pull request as ready for review January 12, 2023 12:10
@elland elland changed the base branch from mls to pcapriotti/subconv-leave January 12, 2023 12:34
@pcapriotti pcapriotti force-pushed the pcapriotti/subconv-leave branch from 24076c0 to 8471e4b Compare January 13, 2023 10:03
@mdimjasevic mdimjasevic changed the base branch from pcapriotti/subconv-leave to mls January 13, 2023 12:29
@mdimjasevic mdimjasevic temporarily deployed to cachix January 13, 2023 13:31 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 13, 2023 13:31 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 13, 2023 13:38 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 13, 2023 13:38 — with GitHub Actions Inactive
@smatting smatting self-requested a review January 16, 2023 11:37
CallsFed 'Galley "on-conversation-updated",
CallsFed 'Galley "on-mls-message-sent",
CallsFed 'Galley "on-new-remote-conversation",
CallsFed 'Galley "on-new-remote-conversation"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I'd avoid adding these calls to the umbrella type 🤔 and instead only add them to the functions that need the constraint.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I cherry-picked this change from the PR you and Paolo have been working on; I suppose you changed this in the meantime so I'll update it here too.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure. I recall the on-mls-message-sent, and on-conversation-updated being there, but I assumed those were always needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are three use sites of the HasProposalActionEffects type alias. If I remove CallsFed 'Galley "on-new-remote-conversation" from this type alias, all the three use sites are missing the CallsFed constraint.


-- FUTUREWORK: remove this check after remote admins are implemented in federation https://wearezeta.atlassian.net/browse/FS-216
foldQualified lconv (\_ -> pure ()) (\_ -> throwS @'MLSUnsupportedProposal) qusr
foldQualified lconvOrSub (\_ -> pure ()) (\_ -> throwS @'MLSUnsupportedProposal) qusr
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be worth it having a different combinator for these cases, like whenLocal and whenRemote, if we're doing that with enough frequency.

@mdimjasevic mdimjasevic temporarily deployed to cachix January 16, 2023 12:27 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 16, 2023 12:27 — with GitHub Actions Inactive
(Epoch 1)
(pscEpoch finalSub)

-- FUTUREWORK: implement the following tests
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think having these stub tests below feels ripe for both merge conflicts and the assumption of tests existing by their outputs being green instead of yellow or something like that,.

@mdimjasevic mdimjasevic temporarily deployed to cachix January 16, 2023 13:01 — with GitHub Actions Inactive
@mdimjasevic mdimjasevic temporarily deployed to cachix January 16, 2023 13:01 — with GitHub Actions Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ok-to-test Approved for running tests in CI, overrides not-ok-to-test if both labels exist

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants