-
Notifications
You must be signed in to change notification settings - Fork 593
chore(tests): end to end testing again #11428
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
Conversation
Co-authored-by: George <[email protected]>
Co-authored-by: George <[email protected]>
Co-authored-by: George <[email protected]>
Co-authored-by: George <[email protected]>
Co-authored-by: George <[email protected]>
Co-authored-by: George <[email protected]>
Co-authored-by: George <[email protected]>
Co-authored-by: George <[email protected]>
| if [ $? -ne 0 ]; then | ||
| EXIT_CODE=1 | ||
| fi | ||
| done |
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.
split tests across circle nodes, tests need to be run and exit code tracked to avoid swallowing and passing even when tests fail
| } else if (shouldSignOut && isLoggedIn) { | ||
| GlobalStore.actions.auth.signOut() | ||
| } | ||
| }, []) |
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.
allow quick log in for faster tests, log out to make sure app is in logged out state, next we can add a linkTo arg to allow quicker feature testing
| version "4.0.4" | ||
| resolved "https://registry.yarnpkg.com/react-native-launch-arguments/-/react-native-launch-arguments-4.0.4.tgz#5cdf12265b50bf98ad05c87f849da99e07ef5500" | ||
| integrity sha512-cB7Sy9m9MX5MvNJliSHEMFWRScSbr95gFuGWnaINBoIK9sP8hloKqhn0vKUHrMQGmNsC1PcpyeiniBqTiU9d5g== | ||
|
|
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.
since we are reusing the beta build this needs to be a proper included dep, but it is small
MounirDhahri
left a comment
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.
LGTM! I am happy we are finally doing this.
The more use this gets in the future, the more we would have to think to run these tests only on merge to main, but that's a problem for another day.
| const args = LaunchArguments.value<MaestroLaunchArguments>() | ||
| const email = args.email | ||
| const password = args.password | ||
| const shouldSignOut = args.shouldSignOut | ||
|
|
||
| if (email && password) { | ||
| GlobalStore.actions.auth.signIn({ |
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.
nice one 😄
I think I will add a helper for this on my CLI for quick login
araujobarret
left a comment
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 for keeping it rolling, so cool to see it running on the CI, and it's green consistently from the past runs!
Started a discussion about reusing the account for some flows, we can move on with the current approach but wanted to raise awareness of that 🦺
| GlobalStore.actions.auth.signOut() | ||
| } | ||
| }, [isHydrated]) | ||
| } |
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 understand the reasoning of this to speed up the tests 100%, timewise it's super expensive, but I'm more defensive of reusing accounts for some tasks because of shared state between test scenarios.
For example, using account X, we enter test scenario 1, which fails and leaves the account in a bad state. Consequently, test scenario 2 also fails due to the current state left by test scenario 1.
Reusing state in e2e tests usually introduces flakiness, so I wonder if we really should do it 🤔
It's a tradeoff of speed vs flakiness, I don't think it's a blocker but my fear is the pipeline gets flaky at some point and we just start ignoring it because the noise 🦺
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 hear the concern and definitely don't want flaky tests either but I am not sure reusing accounts is reasonably avoidable, we are gonna add a huge time cost to every scenario and it will make writing tests more costly because you will need to wait for manual sign up every time or at least account for both signed out and signed in state in your flow writing.
We could possibly:
- have multiple accounts for different flows
- have some guidelines about relying on account state for tests
- have nightly jobs to reset account states or even create new accounts in staging for tests
I am in agreement we want to avoid flakiness as much as possible though.
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, it takes a lot of time just from one account to get to a point where you can go somewhere and test the flow.
I like the idea of having multiple accounts for different flows, about the guidelines is very broad imo, all dev, QA engineers creating scenarios should be aware to avoid any reusage of other scenarios' state, any overflow should be mitigated, but I don't know how to recommend that.
For example, let's say a test where we create a convo, and we have another scenario that opens a convo and reply to something there, the other scenario shouldn't rely on the top conversation to reply, so these kind of things are flags to us reviewers to observe when reviewing any e2e PR, not only Maestro's, Ingetrity too 👍
We can keep the idea of helpers to reset the account state(follows, etc), maybe more stuff to the dev menu so we can have some cleanup when a scenario finishes or breaks, we can evolve this idea as we go, I believe.
Thanks for the response, as I said I liked the ideas you suggested, I think it's def gonna reduce any flakiness level here 💯
| - runFlow: | ||
| commands: | ||
| - tapOn: | ||
| point: 15%,36% |
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.
been there, done that 😢
I can help poking around our checkboxes to make them fully accessible to use with maestro if you want, I totally forgot about it 🫠
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 actually haven't tried updating this since way back, I think George made some changes here recently, will check if there is already a better way
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 did make the checkboxes accessible already
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 checked in maestro studio and still have the problem that all accessible touchables lead to link opening rather than checkbox getting checked, will take a look after this pr
4b8960c
This PR resolves PHIRE-1827
Description
Builds on the work @araujobarret did to get maestro tests running.
Adds a few basic end to end tests using maestro on iOS simulators, running nightly for now.
Adds react native launch args and some init code to enable quick log in and sign out
Here is a successful test run:
https://app.circleci.com/pipelines/github/artsy/eigen/70100/workflows/6de254f2-5ab2-4417-a12c-b071d1bead5b/jobs/244401
Follow-ups
PR Checklist
To the reviewers 👀
Changelog updates
Changelog updates
Cross-platform user-facing changes
iOS user-facing changes
Android user-facing changes
Dev changes
Need help with something? Have a look at our docs, or get in touch with us.