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

add integration test for send #18910

Closed
wants to merge 5 commits into from
Closed

Conversation

J-Son89
Copy link
Contributor

@J-Son89 J-Son89 commented Feb 19, 2024

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.

@status-im-auto
Copy link
Member

status-im-auto commented Feb 19, 2024

Jenkins Builds

Click to see older builds (34)
Commit #️⃣ Finished (UTC) Duration Platform Result
1951d54 #1 2024-02-19 20:43:06 ~1 min tests 📄log
✔️ 1951d54 #1 2024-02-19 20:48:38 ~6 min android 🤖apk 📲
✔️ 1951d54 #1 2024-02-19 20:48:42 ~7 min android-e2e 🤖apk 📲
✔️ 1951d54 #1 2024-02-19 20:49:04 ~7 min ios 📱ipa 📲
6ce90f4 #2 2024-02-21 15:34:02 ~2 min tests 📄log
✔️ 6ce90f4 #2 2024-02-21 15:38:34 ~6 min android-e2e 🤖apk 📲
✔️ 6ce90f4 #2 2024-02-21 15:38:54 ~7 min android 🤖apk 📲
✔️ 6ce90f4 #2 2024-02-21 15:39:40 ~7 min ios 📱ipa 📲
1a44000 #3 2024-02-22 14:53:05 ~2 min tests 📄log
✔️ 1a44000 #3 2024-02-22 14:58:19 ~7 min android 🤖apk 📲
be1e132 #4 2024-02-22 15:00:58 ~2 min tests 📄log
60d0f01 #5 2024-02-22 15:03:23 ~1 min tests 📄log
✔️ 60d0f01 #5 2024-02-22 15:08:35 ~6 min android-e2e 🤖apk 📲
✔️ 60d0f01 #5 2024-02-22 15:09:31 ~7 min android 🤖apk 📲
✔️ 60d0f01 #5 2024-02-22 15:10:04 ~8 min ios 📱ipa 📲
5dcf358 #6 2024-02-22 15:13:24 ~2 min tests 📄log
✔️ 5dcf358 #6 2024-02-22 15:17:33 ~7 min android 🤖apk 📲
✔️ 5dcf358 #6 2024-02-22 15:17:39 ~7 min android-e2e 🤖apk 📲
✔️ 5dcf358 #6 2024-02-22 15:18:52 ~8 min ios 📱ipa 📲
9e33332 #7 2024-02-22 16:06:37 ~3 min tests 📄log
✔️ 9e33332 #7 2024-02-22 16:10:14 ~7 min android 🤖apk 📲
✔️ 9e33332 #7 2024-02-22 16:10:15 ~7 min android-e2e 🤖apk 📲
✔️ 9e33332 #7 2024-02-22 16:11:29 ~8 min ios 📱ipa 📲
b60fc79 #8 2024-02-22 17:06:06 ~3 min tests 📄log
✔️ b60fc79 #8 2024-02-22 17:09:24 ~6 min android-e2e 🤖apk 📲
✔️ b60fc79 #8 2024-02-22 17:09:43 ~7 min android 🤖apk 📲
✔️ b60fc79 #8 2024-02-22 17:11:11 ~8 min ios 📱ipa 📲
b60fc79 #9 2024-02-22 17:11:33 ~3 min tests 📄log
✔️ 1fcf678 #9 2024-02-22 17:20:37 ~7 min android-e2e 🤖apk 📲
✔️ 1fcf678 #9 2024-02-22 17:21:08 ~7 min android 🤖apk 📲
✔️ 1fcf678 #9 2024-02-22 17:21:47 ~8 min ios 📱ipa 📲
✔️ 4bdbda0 #10 2024-02-22 17:31:59 ~7 min android-e2e 🤖apk 📲
✔️ 4bdbda0 #10 2024-02-22 17:32:06 ~7 min android 🤖apk 📲
✔️ 4bdbda0 #10 2024-02-22 17:33:00 ~8 min ios 📱ipa 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
b11650c #12 2024-02-23 15:19:16 ~1 min tests 📄log
✔️ b11650c #11 2024-02-23 15:25:14 ~7 min android-e2e 🤖apk 📲
✔️ b11650c #11 2024-02-23 15:25:16 ~7 min android 🤖apk 📲
✔️ b11650c #11 2024-02-23 15:28:03 ~10 min ios 📱ipa 📲
✔️ fff082e #13 2024-02-26 16:05:47 ~5 min tests 📄log
✔️ fff082e #12 2024-02-26 16:06:23 ~6 min android-e2e 🤖apk 📲
✔️ fff082e #12 2024-02-26 16:07:28 ~7 min android 🤖apk 📲
✔️ fff082e #12 2024-02-26 16:09:49 ~9 min ios 📱ipa 📲

@J-Son89 J-Son89 force-pushed the jc/add-integration-test-for-send branch from 6ce90f4 to 1a44000 Compare February 22, 2024 14:50
@J-Son89 J-Son89 self-assigned this Feb 22, 2024

(defn validate-mnemonic
[mnemonic on-success on-error]
(native-module/validate-mnemonic
Copy link
Contributor Author

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!
Copy link
Contributor Author

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")

Copy link
Contributor Author

Choose a reason for hiding this comment

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

el

@J-Son89 J-Son89 force-pushed the jc/add-integration-test-for-send branch 5 times, most recently from 1fcf678 to 4bdbda0 Compare February 22, 2024 17:24
@@ -9,7 +9,7 @@

(defn enabled? [v] (= "1" v))

(goog-define INFURA_TOKEN "")
(goog-define INFURA_TOKEN "d7aaebc4a3584b5198682ced53801cd3")
Copy link
Contributor

Choose a reason for hiding this comment

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

🤫

Copy link
Contributor Author

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) {
Copy link
Contributor Author

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"
})
Copy link
Contributor Author

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

@J-Son89 J-Son89 force-pushed the jc/add-integration-test-for-send branch from b11650c to fff082e Compare February 26, 2024 15:59
@J-Son89 J-Son89 marked this pull request as ready for review February 26, 2024 15:59
@@ -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$$
Copy link
Contributor Author

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

Copy link
Contributor

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")))
Copy link
Contributor Author

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 :)

Copy link
Contributor

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
Copy link
Contributor Author

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. 🦅

Copy link
Contributor

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?

Copy link
Contributor Author

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)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

remove

Comment on lines +24 to +25
(if (seq error)
(when on-error (on-error error))
Copy link
Contributor

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)
  ...)

Copy link
Contributor Author

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)))
Copy link
Contributor

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?

Copy link
Contributor Author

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 🤔

Copy link
Contributor

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 🤔

Copy link
Contributor Author

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
Copy link
Contributor

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?

@J-Son89 J-Son89 changed the title WIP - Jc/add integration test for send add integration test for send Feb 26, 2024
@ilmotta
Copy link
Contributor

ilmotta commented Mar 1, 2024

@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.

@J-Son89
Copy link
Contributor Author

J-Son89 commented Mar 20, 2024

Closing this as @FFFra & @ilmotta added lots of code which will improve this. Going to handle in a new pr so it's a cleaner review.

@J-Son89 J-Son89 closed this Mar 20, 2024
@J-Son89 J-Son89 deleted the jc/add-integration-test-for-send branch August 29, 2024 10:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants