AES256GCM decrypt: replace Observable with Promise #778
Conversation
bangtoven
force-pushed
the
jungho/decrypt-rxjs-to-promise
branch
from
3ecd20c
to
d242915
Compare
| expect(sampleDataResult).toEqual(value); | ||
| decrypt(cipherText, secret).then( | ||
| value => { | ||
| expect(sampleDataResult).toEqual(JSON.parse(value)); |
There was a problem hiding this comment.
wonder how this has been working
decrypt returns Promise (or used to be Observable) of string, so value (string) and sampleDataResult (object) couldn't be equal, right?
There was a problem hiding this comment.
did the observable resolve to the string or was it an object?
There was a problem hiding this comment.
it was string as well. i haven't changed the internal data type.
There was a problem hiding this comment.
I just logged out what the old test returns when run and it seems like it actually returns an object. Incorrect typing on the decrypt function?
There was a problem hiding this comment.
@vishnumad i am getting a string with old code
There was a problem hiding this comment.
@bangtoven Yup, you're correct it is a string. This test is just broken and will pass no matter what you do in the subscribe function because it's an observable. You can test this by replacing the current expect with expect(true).toEqual(false);. We need to actually block until the observable emits the value.
test("decrypt", done => {
const cipherText =
"06593325a922a928913b5c6ea26f848c4545bcea4e26c4f5ee745316ff22b2780aeccc565730514b2820a94b03f5f89fe7542a35bbdd87a1d52a4352f49482781113db09266c668696778e0a94bc9f866f1e92e7262fd0bb811838284cc64cbc4552b33e9c6fb2582cea4f49471d6d46a16a5c8ac83ee8483ed4dc01f1fde3bfd7a2f173715e0a8d09dd4907483f096a845bff698831ea277c1ca4223d3f6073174cb35119d0a795c1a9cb4f32ee1dcc254d8931";
const sampleDataResult = {
type: "WEB3_RESPONSE",
id: "791fe0ec3dc3de49",
response: {
method: "requestEthereumAccounts",
result: ["0xdf0635793e91d4f8e7426dbd9ed08471186f428d"],
},
};
decrypt(cipherText, secret).subscribe({
next: value => {
expect(sampleDataResult).toEqual(value);
done();
},
});
});There was a problem hiding this comment.
@bangtoven You'll run into a similar issue with the promises in the current test. The test won't actually get marked as failed currently since you're not awaiting the result of the promise.
test("decrypt", async () => {
// ...
const value = await decrypt(cipherText, secret);
expect(value).toEqual(sampleDataResult);
});There was a problem hiding this comment.
@vishnumad hmm. actually i was able to find type error since the test failed.
There was a problem hiding this comment.
@bangtoven It does print the error to console, but doesn't mark the test as failed. See the orange RUNS next to the last line in the screenshot above vs red FAIL below when using await
There was a problem hiding this comment.
oh, gotcha. i just made change to the test code to return the promise.then block.
| fail("expected error"); | ||
| }, | ||
| error => { | ||
| expect(error).toBe("expected error"); |
There was a problem hiding this comment.
not sure how this worked before as well.
error's message is not equal to expected error
| * | ||
| * TODO: Update rxjs for promises |
| return decrypt(encrypted, randSecret).then( | ||
| decrypted => { | ||
| expect(decrypted).toBe("plain text string"); | ||
| }, | ||
| }); | ||
| () => {}, | ||
| ); |
There was a problem hiding this comment.
nit: can we use async/await in tests? A bit easier to read (same below)
* AES256GCM decrypt: replace Observable with Promise * WalletSDKRelay * fix linter * prettier * hmmmmm * rejects.toThrowError * rejects.toThrow * update comment * return promise * use async / await instead * prettier
Summary
How did you test your changes?