Test typescriptification continued by kerryarchibald · Pull Request #8327 · matrix-org/matrix-react-sdk

kerryarchibald · 2022-04-14T14:41:02Z

element-hq/element-web#21965

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

Preview: https://pr8327--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.

…ncryption-test.ts Signed-off-by: Kerry Archibald <kerrya@element.io>

Signed-off-by: Kerry Archibald <kerrya@element.io>

…r-test.ts Signed-off-by: Kerry Archibald <kerrya@element.io>

Signed-off-by: Kerry Archibald <kerrya@element.io>

codecov · 2022-04-14T14:47:25Z

Codecov Report

Merging #8327 (0726c3f) into develop (633229c) will decrease coverage by 0.09%.
The diff coverage is n/a.

❗ Current head 0726c3f differs from pull request most recent head 67f7754. Consider uploading reports for the commit 67f7754 to get more accurate results

@@             Coverage Diff             @@
##           develop    #8327      +/-   ##
===========================================
- Coverage    30.83%   30.74%   -0.10%     
===========================================
  Files          893      892       -1     
  Lines        50781    50706      -75     
  Branches     12923    12906      -17     
===========================================
- Hits         15657    15588      -69     
+ Misses       35124    35118       -6

Impacted Files	Coverage Δ
src/components/views/rooms/SearchBar.tsx	`0.00% <0.00%> (-89.29%)`	⬇️
src/utils/KeyVerificationStateObserver.ts	`81.81% <0.00%> (-9.10%)`	⬇️
src/hooks/useSettings.ts	`81.25% <0.00%> (-6.25%)`	⬇️
...ponents/views/beacon/LeftPanelLiveShareWarning.tsx	`92.30% <0.00%> (-2.14%)`	⬇️
src/components/structures/auth/Registration.tsx	`30.17% <0.00%> (-0.90%)`	⬇️
src/MatrixClientPeg.ts	`30.33% <0.00%> (-0.78%)`	⬇️
src/stores/OwnBeaconStore.ts	`98.16% <0.00%> (-0.51%)`	⬇️
src/events/EventTileFactory.tsx	`66.66% <0.00%> (-0.36%)`	⬇️
src/components/views/rooms/EventTile.tsx	`46.35% <0.00%> (-0.10%)`	⬇️
src/utils/beacon/geolocation.ts	`96.22% <0.00%> (-0.07%)`	⬇️
... and 14 more

turt2live

otherwise lgtm - the questions are legitimate questions, not necessarily hints that things need to change.

turt2live · 2022-04-14T21:35:22Z

-        // This test uses localStorage, clear it beforehand
-        localStorage.clear();
+    // this functionality is commented out in DecryptionFailureTracker
+    // commenting as well as xit here to avoid fixing ts issues for dead code


can we just delete it? (and the code in DecryptionFailureTracker)

we have git history if it needs to be recovered.

Reverted in favour of #8272

turt2live · 2022-04-14T21:36:13Z

+        const error1 = new MockDecryptionError(undefined);
+        const error2 = new MockDecryptionError(undefined);
+        const error3 = new MockDecryptionError(undefined);


I think this was testing the unknown error code behaviour? undefined does aggregate, but so should the 3 stringy error codes (according to the test)

@duxovni is this the correct behaviour? Should DecryptionFailureTracker actually being aggregating the errors my their mapped code before calling the tracking function in trackFailures? Feels like the description of the test and the actual behaviour are at odds

Reverted in favour of #8272

turt2live · 2022-04-14T21:36:39Z

 class MockDecryptionError extends Error {
-    constructor(code) {
+    constructor(
+        public readonly errcode,


needs a type

Reverted in favour of #8272

turt2live · 2022-04-14T21:38:19Z

-        const counts = {};
-        const tracker = new DecryptionFailureTracker(
-            (total, errorCode) => counts[errorCode] = (counts[errorCode] || 0) + total,
-            (errorCode) => Array.from(errorCode).reverse().join(''),


this seemed important to the test?

This was testing that error codes are being run through this map function by asserting the reversed error code was present on the error map expect(counts['1_EDOC_RORRE']).toBe(1)

Because types, and I think because it tests the code in use in our app better, I've tested using the actual error code mapper that the singleton instance uses (

matrix-react-sdk/src/DecryptionFailureTracker.ts

Lines 48 to 60 in 113d69c

}, (errorCode) => {

// Map JS-SDK error codes to tracker codes for aggregation

switch (errorCode) {

case 'MEGOLM_UNKNOWN_INBOUND_SESSION_ID':

return 'OlmKeysNotSentError';

case 'OLM_UNKNOWN_MESSAGE_INDEX':

return 'OlmIndexError';

case undefined:

return 'OlmUnspecifiedError';

default:

return 'UnknownError';

}

});

)
and asserted that MEGOLM_UNKNOWN_INBOUND_SESSION_ID maps to OlmKeysNotSentError and that the track function is called with the mapped error code.

Reverted in favour of #8272

turt2live · 2022-04-14T21:39:37Z

+
+import {
+    MatrixClient,
+    Room } from 'matrix-js-sdk/src/matrix';


Suggested change

Room } from 'matrix-js-sdk/src/matrix';

Room,

} from 'matrix-js-sdk/src/matrix';

Signed-off-by: Kerry Archibald <kerrya@element.io>

duxovni

I already typescriptified test/DecryptionFailureTracker-test.ts in #8272 (along with changing some functionality and adding a bunch of other tests); would you be up for reverting this file, and leaving feedback there if there's anything from this PR I should add into mine?

This reverts commit 1ae748c. Signed-off-by: Kerry Archibald <kerrya@element.io>

This reverts commit 7bd93d0.

…reTracker-test.ts" This reverts commit 1670025.

Signed-off-by: Kerry Archibald <kerrya@element.io>

kerryarchibald · 2022-04-21T07:04:03Z

Thanks @duxovni, I've reverted my changes to DecryptionFailureTracker here in favour of #8272

turt2live

thanks! apologies for the horribly long review (please poke me in a DM if that happens)

sonarqubecloud · 2022-05-02T07:56:01Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

Kerry Archibald added 7 commits April 14, 2022 15:41

test/utils/MegolmExportEncryption-test.js -> test/utils/MegolmExportE…

03fad26

…ncryption-test.ts Signed-off-by: Kerry Archibald <kerrya@element.io>

test/utils/ShieldUtils-test.js - test/utils/ShieldUtils-test.ts

a9745a3

Signed-off-by: Kerry Archibald <kerrya@element.io>

type fixes for ShieldUtils-test

69bd04d

Signed-off-by: Kerry Archibald <kerrya@element.io>

test/DecryptionFailureTracker-test.js -> test/DecryptionFailureTracke…

1670025

…r-test.ts Signed-off-by: Kerry Archibald <kerrya@element.io>

remove unsupported assertion failure messages

7bd93d0

Signed-off-by: Kerry Archibald <kerrya@element.io>

fix ts issues in DecryptionFailureTracker

1ae748c

Signed-off-by: Kerry Archibald <kerrya@element.io>

add mock restores

113d69c

Signed-off-by: Kerry Archibald <kerrya@element.io>

kerryarchibald added the T-Task Refactoring, enabling or disabling functionality, other engineering tasks label Apr 14, 2022

kerryarchibald requested a review from a team as a code owner April 14, 2022 14:41

turt2live suggested changes Apr 14, 2022

View reviewed changes

Kerry Archibald added 6 commits April 19, 2022 09:16

newline

753a998

Signed-off-by: Kerry Archibald <kerrya@element.io>

Merge branch 'develop' into kerry/test-ts-utils

8696a1d

remove commented decriptionfailuretracker code and test

6c47747

Signed-off-by: Kerry Archibald <kerrya@element.io>

make should aggregate error codes correctly pass

641867f

Signed-off-by: Kerry Archibald <kerrya@element.io>

cheaters types in MegolmExportEncryption

876c37e

Signed-off-by: Kerry Archibald <kerrya@element.io>

lint

67c7cd6

Signed-off-by: Kerry Archibald <kerrya@element.io>

duxovni suggested changes Apr 20, 2022

View reviewed changes

Kerry Archibald added 5 commits April 21, 2022 08:58

Merge branch 'develop' into kerry/test-ts-utils

26dd52d

Revert "fix ts issues in DecryptionFailureTracker"

241175a

This reverts commit 1ae748c. Signed-off-by: Kerry Archibald <kerrya@element.io>

Revert "remove unsupported assertion failure messages"

eed8e97

This reverts commit 7bd93d0.

Revert "test/DecryptionFailureTracker-test.js -> test/DecryptionFailu…

259b4e5

…reTracker-test.ts" This reverts commit 1670025.

revert change to DecryptionFailureTracker

b5f3538

Signed-off-by: Kerry Archibald <kerrya@element.io>

kerryarchibald requested review from duxovni and turt2live April 21, 2022 07:04

Merge branch 'develop' into kerry/test-ts-utils

c3c9bdb

duxovni approved these changes Apr 21, 2022

View reviewed changes

Merge branch 'develop' into kerry/test-ts-utils

0726c3f

RiotRobot temporarily deployed to Netlify April 28, 2022 08:49 Destroyed

turt2live approved these changes May 2, 2022

View reviewed changes

Merge branch 'develop' into kerry/test-ts-utils

67f7754

kerryarchibald enabled auto-merge (squash) May 2, 2022 07:51

github-actions Bot temporarily deployed to Netlify May 2, 2022 07:56 Destroyed

kerryarchibald merged commit 4a04be6 into develop May 2, 2022

kerryarchibald deleted the kerry/test-ts-utils branch May 2, 2022 07:57

	}, (errorCode) => {
	// Map JS-SDK error codes to tracker codes for aggregation
	switch (errorCode) {
	case 'MEGOLM_UNKNOWN_INBOUND_SESSION_ID':
	return 'OlmKeysNotSentError';
	case 'OLM_UNKNOWN_MESSAGE_INDEX':
	return 'OlmIndexError';
	case undefined:
	return 'OlmUnspecifiedError';
	default:
	return 'UnknownError';
	}
	});

	Room } from 'matrix-js-sdk/src/matrix';
	Room,
	} from 'matrix-js-sdk/src/matrix';

Uh oh!

Conversation

kerryarchibald commented Apr 14, 2022 • edited by github-actions Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov Bot commented Apr 14, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

turt2live left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duxovni left a comment

Choose a reason for hiding this comment

Uh oh!

kerryarchibald commented Apr 21, 2022

Uh oh!

turt2live left a comment

Choose a reason for hiding this comment

Uh oh!

sonarqubecloud Bot commented May 2, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kerryarchibald commented Apr 14, 2022 •

edited by github-actions Bot

Loading

codecov Bot commented Apr 14, 2022 •

edited

Loading