Separate Integration Tests into their own namespaces #17762
Conversation
|
Hey @DaveWM, and thank you so much for making your first pull request in status-mobile! ❤️ Please help us make your experience better by filling out this brief questionnaire https://goo.gl/forms/uWqNcVpVz7OIopXg2 |
J-Son89
requested review from
ilmotta,
cammellos,
J-Son89,
siddarthkay and
jo-mut
| (rf-test/wait-for [::logout/logout-method])))))) | ||
|
|
||
| (deftest mute-chat-test | ||
| (log/info "========= mute-chat-test ==================") |
There was a problem hiding this comment.
Since the call to info is following a convention to print a headline and it's repeated at the top of all tests, a nice improvement would be to extract to a little helper, like:
(helpers/log-headline :mute-chat-test)| [:chat/one-to-one-chat-created] | ||
| (rf/dispatch-sync [:chat/navigate-to-chat chat-id]) | ||
| (is (= chat-id @(rf/subscribe [:chats/current-chat-id]))) | ||
| (helpers/logout!) |
There was a problem hiding this comment.
For integration tests where things are much more stateful by nature, the exclamation mark convention doesn't add much value to me. The reader already knows a call to (helpers/logout) (no exclamation mark) will cause some side-effect in the system.
| (let | ||
| [compressed-key "zQ3shWj4WaBdf2zYKCkXe6PHxDxNTzZyid1i75879Ue9cX9gA" | ||
| public-key | ||
| "0x048a6773339d11ccf5fd81677b7e54daeec544a1287bd92b725047ad6faa9a9b9f8ea86ed5a226d2a994f5f46d0b43321fd8de7b7997a166e67905c8c73cd37cea" |
There was a problem hiding this comment.
This long line is causing zprint to format the bindings weirdly. You can improve this by extracting public-key to a var or breaking up the string.
(deftest add-contact-test
(let [compressed-key "zQ3shWj4WaBdf2zYKCkXe6PHxDxNTzZyid1i75879Ue9cX9gA"
public-key (str "0x048a6773339d11ccf5fd81677b7e54daeec544a1287"
"bd92b725047ad6faa9a9b9f8ea86ed5a226d2a994f5f4"
"6d0b43321fd8de7b7997a166e67905c8c73cd37cea")
primary-name "zQ3...9cX9gA"]
...))| (rf-test/run-test-async | ||
| (with-app-initialized | ||
| (with-account | ||
| (helpers/assert-messenger-started) |
There was a problem hiding this comment.
For test helpers, since they're so ubiquitous in tests, we have a convention to alias as h, so it would read as (h/assert-messenger-started).
| @@ -0,0 +1,56 @@ | |||
| (ns status-im2.integration-test.helpers | |||
There was a problem hiding this comment.
I think having the test helpers in the integration_test directory is fine and makes sense too, but so far we have added test helpers in src/test_helpers/. If you follow this suggestion, the namespace would be test-helpers.integration.
| (status-im2.integration-test.helpers/create-multiaccount!) | ||
| (rf-test/wait-for | ||
| [:status-im.transport.core/messenger-started] | ||
| ~@body)))) |
There was a problem hiding this comment.
Please, run make lint, I think it will detect some small issues.
| [day8.re-frame.test :as rf-test] | ||
| [re-frame.core :as rf] | ||
| [status-im.events] | ||
| [status-im.multiaccounts.logout.core :as logout] ; so integration tests can run independently |
There was a problem hiding this comment.
These side comments in the ns are unnecessary imo.
| [status-im2.integration-test.helpers :as helpers] | ||
| [status-im2.integration-test.wallet] | ||
| [status-im2.navigation.core] ; so integration tests can run independently | ||
| [status-im2.subs.root] |
There was a problem hiding this comment.
Just status-im2.subs.root is possible (without square brackets) because it's not being aliased.
| (helpers/logout!) | ||
| (rf-test/wait-for [::logout/logout-method]))))) | ||
|
|
||
| (deftest create-community-test |
There was a problem hiding this comment.
Maybe better is to extract this community test outside the core namespace, and into integration_test/community.cljs, just like you did for wallet.cljs and chat.cljs.
| (rf-test/run-test-async | ||
| (with-app-initialized | ||
| (with-account | ||
| (helpers/assert-messenger-started) |
There was a problem hiding this comment.
Every time with-account is used in the tests, assert-messenger-started is called as well. I think it would be better to move this check to the macro itself because it's always desirable.
DaveWM
force-pushed
the
develop
branch
3 times, most recently
from
b973fda to
9f9384b
Compare
|
Thanks for the review @ilmotta. All your suggestions sound good to me, I've updated the PR to implement them all |
Jenkins BuildsClick to see older builds (16)
|
DaveWM
force-pushed
the
develop
branch
2 times, most recently
from
5b71e25 to
90f2a99
Compare
89% of end-end tests have passedFailed tests (2)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (3)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (40)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
|
DaveWM
force-pushed
the
develop
branch
2 times, most recently
from
25cfe52 to
d0baaad
Compare
Fixes #17679
Summary
This PR separates the integration tests into separate namespaces under
status-im2.integration-test. It also adds some helper macros for performing setup steps.Review notes
I've made a few small changes from the brief in the issue.
Firstly I've put the integration tests in the
status-im2/integration-testdirectory, rather than have a core namespace atsrc/status_im2/integration_test.cljsand the other tests elsewhere. I thought this was best so that all the integration tests are in a single directory, but I can easily change this if required.Secondly, the issue says to re-use state from some tests to run others. Doing this to the letter would require setting up some form of dependency graph for the tests, which I thought would be difficult to maintain. Also, it means you often can't run a single test, as it may depend on other tests. I've therefore taken the slightly different approach of having some setup functions that first check if they've already been run, and if they have then just skip the setup entirely. I think this keeps to the spirit of what was requested in the issue, and it's also more maintainable. Happy to incorporate any feedback though :)
Testing notes
None
Platforms
N/A
Areas that maybe impacted
Only tests.
Functional
No changes
Non-functional
No changes
Steps to test
Run the tests.
Before and after screenshots comparison
N/A
status: ready