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

test(utils): remove duplicates #1239

Merged
merged 4 commits into from
Nov 14, 2023
Merged

Conversation

alexandre-abrioux
Copy link
Member

Description of the changes

  • removed duplicate encryption/decryption tests
  • removed "error" comments on tests that should not fail

// 'getAddressFromPrivateKey() error'
expect(identity).toBe(rawId.address);
Copy link
Member Author

@alexandre-abrioux alexandre-abrioux Nov 14, 2023

Choose a reason for hiding this comment

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

Those comments were messages displayed in case of a failure back when we were still using mocha instead of jest, they were placed as the second argument in the expect, see #283. They can be removed, at least for happy-path tests, they can be confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I removed them for failure tests too, see #1239 (comment)

Comment on lines -177 to -192
it('can encrypt()', async () => {
const encryptedData = await ecEncrypt(rawId.publicKey, anyData);
// 'encrypt() error'
expect(encryptedData.length).toBe(226);
// 'decrypt() error'
expect(await ecDecrypt(rawId.privateKey, encryptedData)).toBe(anyData);
});

it('can decrypt()', async () => {
const data = await ecDecrypt(
rawId.privateKey,
'307bac038efaa5bf8a0ac8db53fd4de8024a0c0baf37283a9e6671589eba18edc12b3915ff0df66e6ffad862440228a65ead99e3320e50aa90008961e3d68acc35b314e98020e3280bf4ce4258419dbb775185e60b43e7b88038a776a9322ff7cb3e886b2d92060cff2951ef3beedcc70a',
);
// 'decrypt() error'
expect(data).toBe(anyData);
});
Copy link
Member Author

@alexandre-abrioux alexandre-abrioux Nov 14, 2023

Choose a reason for hiding this comment

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

Those two tests are duplicates see above lines 95 and 118 (from the new line numbers)

@alexandre-abrioux alexandre-abrioux marked this pull request as ready for review November 14, 2023 09:46
@alexandre-abrioux alexandre-abrioux enabled auto-merge (squash) November 14, 2023 09:46
Comment on lines 34 to 33
// 'getAddressFromPrivateKey() error'
// getAddressFromPrivateKey() error
Copy link
Member Author

Choose a reason for hiding this comment

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

In case of failure tests, I kept the comments but removed the quotes

Copy link
Contributor

Choose a reason for hiding this comment

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

these comments were generated when migrating from mocha to jest, they can totally be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

OK then, I'm removing them altogether

@coveralls
Copy link

Coverage Status

coverage: 86.918%. remained the same
when pulling cc437e7 on test-remove-duplicates
into b341203 on master.

Comment on lines 34 to 33
// 'getAddressFromPrivateKey() error'
// getAddressFromPrivateKey() error
Copy link
Contributor

Choose a reason for hiding this comment

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

these comments were generated when migrating from mocha to jest, they can totally be removed

@alexandre-abrioux alexandre-abrioux merged commit 71b2c46 into master Nov 14, 2023
26 checks passed
@alexandre-abrioux alexandre-abrioux deleted the test-remove-duplicates branch November 14, 2023 11:11
@MantisClone
Copy link
Member

Nice work.

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.

5 participants