Update README for UI Tests #21594
Merged
Update README for UI Tests #21594
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
|
| App Name |  WordPress Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr21594-3eae490 | |
| Version | 23.2 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 3eae490 | |
| App Center Build | WPiOS - One-Offs #7141 |
Contributor
|
| App Name |  Jetpack Alpha |
|
| Configuration | Release-Alpha | |
| Build Number | pr21594-3eae490 | |
| Version | 23.2 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 3eae490 | |
| App Center Build | jetpack-installable-builds #6182 |
mokagio
reviewed
Sep 18, 2023
WordPress/UITests/README.md
Outdated
| * 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) |
Contributor
There was a problem hiding this comment.
I'm not familiar with the .../ syntax, is that a standard way to say "a older defined somewhere"?
Contributor
Author
There was a problem hiding this comment.
oh that was to say that it's in that folder, would just /Tests be clearer?
Contributor
There was a problem hiding this comment.
I'd vote for just using the folder name, w/o ... or /, to avoid making it look like a relative reference.
pachlava
reviewed
Sep 18, 2023
WordPress/UITests/README.md
Outdated
| 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. | ||
| 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.j |
Contributor
There was a problem hiding this comment.
Suggested change
| 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.j | |
| 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. |
pachlava
reviewed
Sep 18, 2023
WordPress/UITests/README.md
Outdated
| 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. | ||
|
|
||
| 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: |
Contributor
There was a problem hiding this comment.
- From seeing the
... in the logs:it looks like you've intended to provide an error sample. - 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?
Contributor
Author
There was a problem hiding this comment.
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?
Contributor
There was a problem hiding this comment.
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)
pachlava
reviewed
Sep 18, 2023
WordPress/UITests/README.md
Outdated
Comment on lines
123
to
124
| 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) |
Contributor
There was a problem hiding this comment.
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?
Contributor
Author
There was a problem hiding this comment.
good point, yeah looks like ok to remove without losing context.
WordPress/UITests/README.md
Outdated
| * 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)). |
Contributor
There was a problem hiding this comment.
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)). |
WordPress/UITests/README.md
Outdated
| ## 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()` |
Contributor
There was a problem hiding this comment.
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()` |
WordPress/UITests/README.md
Outdated
|
|
||
| ## 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). |
Contributor
There was a problem hiding this comment.
- Nitpick:
Testis capitalized andscreenis not. - 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. |
Contributor
Author
There was a problem hiding this comment.
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.
WordPress/UITests/README.md
Outdated
|
|
||
| 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). | ||
|
|
||
| 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. |
Contributor
There was a problem hiding this comment.
- Possibly there should be
that - Providing more context (
test methodvsmethod) 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. |
| 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. |
Contributor
There was a problem hiding this comment.
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 🙂)
Contributor
Author
There was a problem hiding this comment.
that is a pretty valid suggestion though 😆
WordPress/UITests/README.md
Outdated
|
|
||
| ## 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) | ||
| * 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) |
Contributor
There was a problem hiding this comment.
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). |
WordPress/UITests/README.md
Outdated
| 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) |
Contributor
There was a problem hiding this comment.
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). |
Contributor
Author
|
@pachlava thank you for the meticulous review as always 🙇 i made the updates, and in the process realized how important proofreading especially with fresh eyes is 😅 can you help take another look? thanks in advance! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Updates the README file for UI Tests to include the current structure and recommendation when adding new UI tests. Also extended the mock section to include steps on adding scenarios for stateful behavior.
Testing
Only README file updated. As long as CI is 🟢, should be enough.