Skip to content
This repository has been archived by the owner on Sep 11, 2024. It is now read-only.

Add test utils for Beacons #8064

Merged
merged 6 commits into from
Mar 16, 2022
Merged

Conversation

kerryarchibald
Copy link
Contributor

@kerryarchibald kerryarchibald commented Mar 16, 2022


This change is marked as an internal change (Task), so will not be included in the changelog.

Preview: https://pr8064--matrix-react-sdk.netlify.app
⚠️ Do you trust the author of this PR? Maybe this build will steal your keys or give you malware. Exercise caution. Use test accounts.

Kerry Archibald added 5 commits March 16, 2022 11:07
Signed-off-by: Kerry Archibald <kerrya@element.io>
Signed-off-by: Kerry Archibald <kerrya@element.io>
Signed-off-by: Kerry Archibald <kerrya@element.io>
Signed-off-by: Kerry Archibald <kerrya@element.io>
@kerryarchibald kerryarchibald added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Mar 16, 2022
@kerryarchibald kerryarchibald requested a review from a team as a code owner March 16, 2022 10:12
@codecov
Copy link

codecov bot commented Mar 16, 2022

Codecov Report

Merging #8064 (0d9c377) into develop (26e6f8d) will not change coverage.
The diff coverage is n/a.

❗ Current head 0d9c377 differs from pull request most recent head f201259. Consider uploading reports for the commit f201259 to get more accurate results

@@           Coverage Diff            @@
##           develop    #8064   +/-   ##
========================================
  Coverage    27.02%   27.02%           
========================================
  Files          866      866           
  Lines        52015    52015           
  Branches     13185    13185           
========================================
  Hits         14055    14055           
  Misses       37960    37960           

Copy link
Contributor

@andybalaam andybalaam left a comment

Choose a reason for hiding this comment

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

A few small comments, but looks good.

...contentProps,
};
const event = new MatrixEvent({
type: `${M_BEACON_INFO.name}.${sender}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer e.g. .1 instead of .${sender} just because I don't want to propagate the confusing mistake made in the MSC.

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 think in practice and here we should use something like .${sender}.${counter} to have the likelihood of colliding event types much lower. Added a counter

};

type ContentProps = {
uri: string;
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 prefer a name like geouri to make it clear it's not just any URI.

});
* ```
*/
export const getMockClientWithEventEmitter = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this name mention that we are overriding MatrixClientPeg.get as well as returning the mock?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's already a pretty long function name, the comment mentions that it mocks the peg.

Signed-off-by: Kerry Archibald <kerrya@element.io>
@kerryarchibald kerryarchibald enabled auto-merge (squash) March 16, 2022 13:04
@kerryarchibald kerryarchibald merged commit e677901 into develop Mar 16, 2022
@kerryarchibald kerryarchibald deleted the psf-662/beacon-test-utils branch March 16, 2022 13:11
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
T-Task Refactoring, enabling or disabling functionality, other engineering tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants