Skip to content

Update README for UI Tests #21594

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Sep 19, 2023
Merged

Update README for UI Tests #21594

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f1e7f06
update ui test readme
jostnes Sep 18, 2023
f9f7f13
remove ... from folder links
jostnes Sep 18, 2023
1c3cbad
add missing screenshot, add clarifying statement
jostnes Sep 18, 2023
3eae490
review updates
jostnes Sep 19, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 33 additions & 6 deletions WordPress/UITests/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -85,17 +85,44 @@ Note that due to the mock server setup, tests cannot be run on physical devices

When adding a new UI test, consider:

* Whether you need to test a user flow (to accomplish a task or goal) or a specific feature (e.g. boundary testing).
* What screens are being tested (defined as page objects in `Screens/`).
* Whether there are repeated flows across tests (defined in `Flows/`).
* What network requests are made during the test (defined in `API-Mocks/`).
* Whether to test a user flow (to accomplish a task or goal) or a specific feature (e.g. boundary testing).
* What screens are being tested (defined as screen objects in [Screens](https://github.com/wordpress-mobile/WordPress-iOS/tree/trunk/WordPress/UITestsFoundation/Screens)).
* Whether there are repeated flows across tests (defined in [Flows](https://github.com/wordpress-mobile/WordPress-iOS/tree/trunk/WordPress/UITests/Flows)).
Copy link
Contributor

@pachlava pachlava Sep 18, 2023

Choose a reason for hiding this comment

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

Just for the sake or pronounciation:

Suggested change
* Whether there are repeated flows across tests (defined in [Flows](https://github.com/wordpress-mobile/WordPress-iOS/tree/trunk/WordPress/UITests/Flows)).
* Are there repeated flows across tests (defined in [Flows](https://github.com/wordpress-mobile/WordPress-iOS/tree/trunk/WordPress/UITests/Flows)).

* What network requests are made during the test (defined in [API-Mocks](https://github.com/wordpress-mobile/WordPress-iOS/tree/trunk/API-Mocks)).

It's preferred to focus UI tests on entire user flows, and group tests with related flows or goals in the same test suite.
Tests classes are grouped together in [Tests](https://github.com/wordpress-mobile/WordPress-iOS/tree/trunk/WordPress/UITests/Tests)

When you add a new test, you may need to add new screens, methods, and flows. We use page objects and method chaining for clarity in our tests. Wherever possible, use an existing `accessibilityIdentifier` (or add one to the app) instead of a string to select a UI element on the screen. This ensures tests can be run regardless of the device language.

## Naming convention

* When creating new tests, use this format for the name to make it easier to see what the test is doing: `testActionFeature` e.g. `testCreateScheduledPost()`
* When creating new methods, use this format: `actionObject` e.g. `closePostSettings()`
Copy link
Contributor

@pachlava pachlava Sep 18, 2023

Choose a reason for hiding this comment

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

Subjective: for 1-3, to me, it looks like separating template from example makes it easier to understand:

Suggested change
* When creating new methods, use this format: `actionObject` e.g. `closePostSettings()`
* When creating new methods, use the `actionObject` format, e.g. `closePostSettings()`

* For assert methods, use this format `verifyWhatToVerify` e.g. `verifyPostExists()`
* Note that there’s a common global method `assertScreenIsLoaded()` that can be used to assert all screens

## Passing hard-coded `Strings` | `Numbers` in tests

There are some cases where we would need to pass hard-coded values in the test, this should happen on the Test level and not on the screen level (unless there’s a really good reason).
Copy link
Contributor

@pachlava pachlava Sep 18, 2023

Choose a reason for hiding this comment

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

  1. Nitpick: Test is capitalized and screen is not.
  2. Does the (unless there’s a really good reason). serves a good purpose?
Suggested change
There are some cases where we would need to pass hard-coded values in the test, this should happen on the Test level and not on the screen level (unless there’s a really good reason).
There are some cases where we would need to pass hard-coded values in the test, this should happen on the Test level and not on the Screen level.

Copy link
Contributor Author

@jostnes jostnes Sep 19, 2023

Choose a reason for hiding this comment

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

Does the (unless there’s a really good reason). serves a good purpose?

yeah we can drop this to not encourage it, even if it needs to happen it will be explained in the PR.


This is so methods are not limited to being used with a fixed value and remain flexible. In the case where those values change we would also be able to update only the test file(s) without making changes elsewhere.
Copy link
Contributor

@pachlava pachlava Sep 18, 2023

Choose a reason for hiding this comment

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

  1. Possibly there should be that
  2. Providing more context (test method vs method) makes it easier to grasp that it's about methods in Screen files, IMO.
Suggested change
This is so methods are not limited to being used with a fixed value and remain flexible. In the case where those values change we would also be able to update only the test file(s) without making changes elsewhere.
This is so that test methods are not limited to being used with a fixed value and remain flexible. In the case where those values change we would also be able to update only the test file(s) without making changes elsewhere.


## Adding or updating network mocks

When you add a test (or when the app changes), the request definitions for WireMock need to be updated in `API-Mocks/`. You can read WireMock’s documentation [here](http://wiremock.org/docs/).

If you are unsure what network requests need to be mocked for a test, an easy way to find out is to run the app through [Charles Proxy](https://www.charlesproxy.com/) and observe the required requests.
If you are unsure what network requests need to be mocked for a test, an easy way to find out is to run the app through [Proxyman](https://proxyman.io/) or [Charles Proxy](https://www.charlesproxy.com/) and observe the required requests.
Copy link
Contributor

@pachlava pachlava Sep 18, 2023

Choose a reason for hiding this comment

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

Suggested change
If you are unsure what network requests need to be mocked for a test, an easy way to find out is to run the app through [Proxyman](https://proxyman.io/) or [Charles Proxy](https://www.charlesproxy.com/) and observe the required requests.
If you are unsure what network requests need to be mocked for a test, a way to find this out is to run the app through [Proxyman](https://proxyman.io/) or [Charles Proxy](https://www.charlesproxy.com/) and observe the required requests.

(No, just joking 🙂)

Copy link
Contributor Author

@jostnes jostnes Sep 19, 2023

Choose a reason for hiding this comment

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

that is a pretty valid suggestion though 😆


Currently, the project does not apply strict mock matching criteria, this means that if there are unmatched requests that are not being used by the test itself, the test should still work although errors like this can be seen in the logs:
Copy link
Contributor

@pachlava pachlava Sep 18, 2023

Choose a reason for hiding this comment

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

  1. From seeing the ... in the logs: it looks like you've intended to provide an error sample.
  2. Maybe it's just me, but this paragraph might indirectly encourage to not keep the "not directly related" mocks in shape 🤔 Should it really be there, and does it make the overall approach better or cleaner?

Copy link
Contributor Author

@jostnes jostnes Sep 18, 2023

Choose a reason for hiding this comment

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

From seeing the ... in the logs: it looks like you've intended to provide an error sample.

oh yes, you're right, the screenshot didn't get copied when i copied the rest of the doc 😓 i'll add that in.

this paragraph might indirectly encourage to not keep the "not directly related" mocks in shape

the reason i added that is that there are existing mock calls that are still not matched (not many but there are) that will appear in the logs, so that would serve as a statement as to why it's happening and reduces the potential of having contributors ask about it. considering the endpoints are not used by the test, i would say that the approach doesn't impact the test itself but does clutter the console logs with "Request was not matched" logs.

i personally think we should keep this explanatory statement. i also added a recommendation so it doesn't look like we're encouraging keeping the mock requests unmatched in 1c3cbad can you take another look and see if it reads better now?

Copy link
Contributor

@pachlava pachlava Sep 18, 2023

Choose a reason for hiding this comment

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

Yes, it's better now, thank you, Jos 🙇 (I'll provide my further comments all at once as a part of review shortly, sorry for cluttering the PR with separate standalone ones)


## Using stateful behavior for mocks
1. To add scenarios that use stateful behavior, do the following:
Add the new scenario in [scenarios.json](https://github.com//wordpress-mobile/WordPress-iOS/tree/trunk/API-Mocks/WordPressMocks/src/main/assets/mocks/__files/__admin/scenarios.json)
Copy link
Contributor

@pachlava pachlava Sep 18, 2023

Choose a reason for hiding this comment

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

Possibly you meant starting the list in the following way, and using the first sentence as an introduction?

Suggested change
1. To add scenarios that use stateful behavior, do the following:
Add the new scenario in [scenarios.json](https://github.com//wordpress-mobile/WordPress-iOS/tree/trunk/API-Mocks/WordPressMocks/src/main/assets/mocks/__files/__admin/scenarios.json)
To add scenarios that use stateful behavior, do the following:
1. Add the new scenario in [scenarios.json](https://github.com//wordpress-mobile/WordPress-iOS/tree/trunk/API-Mocks/WordPressMocks/src/main/assets/mocks/__files/__admin/scenarios.json)

P.S. Since the header text is very similar to the first sentence, WDYT of removing the first sentence?

Copy link
Contributor Author

@jostnes jostnes Sep 19, 2023

Choose a reason for hiding this comment

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

good point, yeah looks like ok to remove without losing context.

2. Fetch and reset scenario during `SetUp()` in the test class containing the test, e.g. seen on [Notification Test](https://github.com/wordpress-mobile/WordPress-iOS/blob/5730cee6568fe43fb3a5108e396e12244c62b3e5/WordPress/UITests/Tests/NotificationTests.swift#L18-L23)
3. Update JSON mappings to contain the following 3 new attributes, `scenarioName`, `requiredScenarioState` and `newScenarioState`, and the response matching the state of the scenario, e.g. seen on [Notification Test](https://github.com/wordpress-mobile/WordPress-iOS/blob/5730cee6568fe43fb3a5108e396e12244c62b3e5/API-Mocks/WordPressMocks/src/main/assets/mocks/mappings/wpcom/notifications/notifications_comment_reply_before.json#L2-L4)

## Tips and tricks on using mocks
* When getting the same request with the same header but a different request body to return different responses, experiment with using [different matchers](https://docs.wiremock.io/request-matching/matcher-types/). From some experimenting, would recommend using `matchesJsonPath` which is used to differentiate [Create Page](https://github.com/wordpress-mobile/WordPress-iOS/blob/5a00e849d8877e8ae2a6ec6bc9c762e68e6e0620/API-Mocks/WordPressMocks/src/main/assets/mocks/mappings/wpcom/pages/sites_106707880_pages_new.json#L18) and [Create Post](https://github.com/wordpress-mobile/WordPress-iOS/blob/5a00e849d8877e8ae2a6ec6bc9c762e68e6e0620/API-Mocks/WordPressMocks/src/main/assets/mocks/mappings/wpcom/posts/posts_new.json#L18)
Copy link
Contributor

@pachlava pachlava Sep 18, 2023

Choose a reason for hiding this comment

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

Suggested change
* When getting the same request with the same header but a different request body to return different responses, experiment with using [different matchers](https://docs.wiremock.io/request-matching/matcher-types/). From some experimenting, would recommend using `matchesJsonPath` which is used to differentiate [Create Page](https://github.com/wordpress-mobile/WordPress-iOS/blob/5a00e849d8877e8ae2a6ec6bc9c762e68e6e0620/API-Mocks/WordPressMocks/src/main/assets/mocks/mappings/wpcom/pages/sites_106707880_pages_new.json#L18) and [Create Post](https://github.com/wordpress-mobile/WordPress-iOS/blob/5a00e849d8877e8ae2a6ec6bc9c762e68e6e0620/API-Mocks/WordPressMocks/src/main/assets/mocks/mappings/wpcom/posts/posts_new.json#L18)
* When getting the same request with the same header but a different request body to return different responses, experiment with using [different matchers](https://docs.wiremock.io/request-matching/matcher-types/). From some experimenting, I would recommend using `matchesJsonPath` which is used to differentiate [Create Page](https://github.com/wordpress-mobile/WordPress-iOS/blob/5a00e849d8877e8ae2a6ec6bc9c762e68e6e0620/API-Mocks/WordPressMocks/src/main/assets/mocks/mappings/wpcom/pages/sites_106707880_pages_new.json#L18) and [Create Post](https://github.com/wordpress-mobile/WordPress-iOS/blob/5a00e849d8877e8ae2a6ec6bc9c762e68e6e0620/API-Mocks/WordPressMocks/src/main/assets/mocks/mappings/wpcom/posts/posts_new.json#L18).

* Use `verbose` to debug errors, this can be updated adding the verbose parameter when [starting the WireMock server](https://github.com/wordpress-mobile/WordPress-iOS/blob/5a00e849d8877e8ae2a6ec6bc9c762e68e6e0620/API-Mocks/scripts/start.sh#L20-L23) (don’t forget the slash to not break the command)
Copy link
Contributor

@pachlava pachlava Sep 18, 2023

Choose a reason for hiding this comment

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

Possibly you meant:

Suggested change
* Use `verbose` to debug errors, this can be updated adding the verbose parameter when [starting the WireMock server](https://github.com/wordpress-mobile/WordPress-iOS/blob/5a00e849d8877e8ae2a6ec6bc9c762e68e6e0620/API-Mocks/scripts/start.sh#L20-L23) (don’t forget the slash to not break the command)
* Use `verbose` to debug errors, this can be updated by adding the `verbose` parameter when [starting the WireMock server](https://github.com/wordpress-mobile/WordPress-iOS/blob/5a00e849d8877e8ae2a6ec6bc9c762e68e6e0620/API-Mocks/scripts/start.sh#L20-L23) (don’t forget the slash to not break the command).

* If there are no errors on the console, but the mocks don’t work as expected, check out the app’s logs for errors. Sometimes it could be that the JSON mapping is not parsed correctly.