Conversation
hansl
force-pushed
the
hansl/node-e2e
branch
3 times, most recently
from
b38bb7d to
dd1f9f9
Compare
|
Don't get scared by the amount of lines, the |
pikajude
left a comment
pikajude
left a comment
There was a problem hiding this comment.
LGTM, assuming it passes CI :)
| @@ -0,0 +1,71 @@ | |||
| { | |||
There was a problem hiding this comment.
Is it worth deleting all the commented out lines here? I think the tsconfig format is well documented and this is pretty hard to read.
There was a problem hiding this comment.
I'm okay with either. I just like to search for the field and see if it's enabled, but yeah it's pretty well documented and every IDE have completion for it.
basvandijk
left a comment
basvandijk
left a comment
There was a problem hiding this comment.
The Nix changes look good sans some minor bugs.
chenyan-dfinity
left a comment
chenyan-dfinity
left a comment
There was a problem hiding this comment.
IIRC, we are only testing the counter canister in node with a hand-written IDL-JS binding. What's preventing us from calling dfx build in node?
src/userlib/js/src/actor.ts
Outdated
| switch (status.status) { | ||
| case RequestStatusResponseStatus.Replied: { | ||
| return decodeReturnValue(returnType, status.reply.arg); | ||
| return status.reply && decodeReturnValue(returnType, status.reply.arg); |
There was a problem hiding this comment.
Should we throw if there is no status.reply? Also does this mean this will return a boolean instead of the actual return value?
There was a problem hiding this comment.
It will return null if reply is null. I should investigate this as it was working on Friday but changed today.
There was a problem hiding this comment.
I think we need to throw. Return value can also be null, and we want to distinguish a proper return from failure.
There was a problem hiding this comment.
This is a break from the public spec. So I'm not sure what happened but I'm investigating and it happened also in the rust code (see https://dfinity.slack.com/archives/CGA566TPV/p1583785482375400). I'll revert this line and look it up.
|
@chenyan-dfinity yes, that is correct. It’s a framework to add more tests. |
|
Can we call |
|
@chenyan-dfinity We're sharing >90% of the code already. We will need more stuff to use dfx projects, but it's out of scope of this PR and should be separate work. |
|
Could you describe a bit more about the framework? I think I get the idea here, but I am not sure we get any good view of what the end result/framework would look like. |
5 participants
Includes #435.