-
Notifications
You must be signed in to change notification settings - Fork 984
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
add integration test for send #18910
Conversation
Jenkins BuildsClick to see older builds (34)
|
6ce90f4
to
1a44000
Compare
|
||
(defn validate-mnemonic | ||
[mnemonic on-success on-error] | ||
(native-module/validate-mnemonic |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this over the reframe event that we have because the reframe event is coupled with an options menu opening up.
Additionally I thought it best not to add re-frame events just for the sake of testing.
@@ -42,16 +84,6 @@ | |||
[] | |||
(is (= @(rf/subscribe [:communities/create]) constants/community))) | |||
|
|||
(defn create-new-account! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wasn't used
@@ -5,3 +5,11 @@ | |||
(def community {:membership 1 :name "foo" :description "bar"}) | |||
|
|||
(def account-name "account-abc") | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
el
1fcf678
to
4bdbda0
Compare
@@ -9,7 +9,7 @@ | |||
|
|||
(defn enabled? [v] (= "1" v)) | |||
|
|||
(goog-define INFURA_TOKEN "") | |||
(goog-define INFURA_TOKEN "d7aaebc4a3584b5198682ced53801cd3") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🤫
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will need to remove this infura key and add another. Will address this 👍
@@ -670,6 +670,36 @@ void _ToChecksumAddress(const FunctionCallbackInfo<Value>& args) { | |||
|
|||
} | |||
|
|||
void _LoginAccount(const FunctionCallbackInfo<Value>& args) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cammellos I just copied this code and pieced together what I think is correct.
:seed-phrase "destroy end torch puzzle develop note wise island disease chaos kind bus" | ||
:key-uid "0x6827f3d1e21ae723a24e0dcbac1853b5ed4d651c821a2326492ce8f2367ec905" | ||
:public-key "0x48b8b9e2a8f71e1beae729d83bcd385ffc6af0d5" | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public key is not needed but handy to have easily available so we can ensure the balance is alright etc
b11650c
to
fff082e
Compare
@@ -359,7 +359,7 @@ test-watch-for-repl: ##@test Watch all Clojure tests and support REPL connection | |||
"until [ -f $$SHADOW_OUTPUT_TO ] ; do sleep 1 ; done ; node --require ./test-resources/override.js $$SHADOW_OUTPUT_TO --repl" | |||
|
|||
test-unit: export SHADOW_OUTPUT_TO := target/unit_test/test.js | |||
test-unit: export SHADOW_NS_REGEXP := ^(?!tests\.integration-test)(?!tests-im\.contract-test).*-test$$ | |||
test-unit: export SHADOW_NS_REGEXP := ^(?!tests\.integration-test)(?!tests\.contract-test).*-test$$ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
copied from @ilmotta's pr
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry it took me a while to merge because I thought I had it in. You should be able to rebase with the fix now.
@@ -167,5 +167,5 @@ | |||
|
|||
(def default-kdf-iterations 3200) | |||
|
|||
(def community-accounts-selection-enabled? true) | |||
(def community-accounts-selection-enabled? (enabled? (get-config :ACCOUNT_SELECTION_ENABLED "0"))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not sure why this is here but we should move to feature flags :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's here because it was created before you created the new feature flags solution. Should be moved definitely.
|
||
(def send-amount "0.0001") | ||
|
||
(deftest wallet-send-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is one of the main focuses of this pr. 🦅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems good @J-Son89
I'd like to have a clearer way to read it, but it seems the re-frame's macro wait-for
is making the reading harder.
Looking at the implementation of that macro, it returns:
;; ...
(wait-for* ~ids ~failure-ids (fn [~event-sym] ~@body))
and wait-for*
is a function, we could probably use it in our own macro for testing.
@ilmotta wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah the wait for makes it ugly to write too.
one other feature that would be nice, often we want to wait on multiple keys and in order, but we don't support this.
we can only wait for multiple but once one is satisfied then it moves forward
@@ -70,6 +70,12 @@ | |||
(callback (.openAccounts native-status test-dir))) | |||
:createAccountAndLogin | |||
(fn [request] (.createAccountAndLogin native-status request)) | |||
:restoreAccountAndLogin | |||
(fn [request] | |||
(prn native-status) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove
(if (seq error) | ||
(when on-error (on-error error)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we can use and
(if (and (seq error) on-error)
(on-error error)
...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
(rf-test/wait-for | ||
[:wallet/suggested-routes-success] | ||
(let [route (rf/sub [:wallet/wallet-send-route])] | ||
(is (true? (some? route))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we don't need that true?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is an early break on the test if no route is found. I can remove but I think it adds value 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I think it adds value
If it's intentional it's ok 👍
this is an early break on the test if no route is found
I'm not getting it, could you please expand more on the "early break"?
I think it doesn't matter if one check (call to is
) fails, the rest of the test is still executed 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah true, I still think it's worth checkn the route was found, it will help in the CI to know if it failed because of no route found!
|
||
(def send-amount "0.0001") | ||
|
||
(deftest wallet-send-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems good @J-Son89
I'd like to have a clearer way to read it, but it seems the re-frame's macro wait-for
is making the reading harder.
Looking at the implementation of that macro, it returns:
;; ...
(wait-for* ~ids ~failure-ids (fn [~event-sym] ~@body))
and wait-for*
is a function, we could probably use it in our own macro for testing.
@ilmotta wdyt?
@ulisesmac, @J-Son89, once I get more approvals in PR #19025 I'll merge it right away. Please, have a look when you can in that PR because it solves the pain points both of you mentioned in this PR. |
This test is to cover the basic send flow.
In this test it:
recover account with balance
enable testnet
navigates through the send flow happy path and sends a small transaction to itself.
The balance diminished should be very low, I will keep adding test eth to this account daily,
an alternative is to also test using on arbitrum or optimism as fees will naturally be lower.