-
Notifications
You must be signed in to change notification settings - Fork 93
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
Acceptance test doc update #297
Conversation
@jnvtnguyen PTAL. |
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.
Made a partial review.
Acceptance-Tests.md
Outdated
|
||
In this example, the `discardCurrentChanges()` function checks if the viewport width corresponds to a mobile device, and if so, clicks on the mobile-specific discard button. Otherwise, it clicks on the desktop-specific discard button. Finally, it confirms the discard action. This approach allows us to maintain a single set of tests while accommodating differences between desktop and mobile environments. | ||
|
||
### console errors logging functionality in Acceptance Tests |
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.
Move this section (console errors section) to above since it can be used in all cases (mobile/desktop), also should be capitalized beginning word and rest non capitalized. Console errors...
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.
done, Thanks :)
Acceptance-Tests.md
Outdated
|
||
### console errors logging functionality in Acceptance Tests | ||
|
||
Now, Acceptance Tests are capable of finding console errors during CUJ's and it will break the test. But in some cases where the console error can be ignored and not in high priority right now then we can ignore that with help of `consoleReporter.setConsoleErrorsToIgnore` |
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.
consoleReporter -> ConsoleReporter
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.
Done, thanks :)
Acceptance-Tests.md
Outdated
|
||
Now, Acceptance Tests are capable of finding console errors during CUJ's and it will break the test. But in some cases where the console error can be ignored and not in high priority right now then we can ignore that with help of `consoleReporter.setConsoleErrorsToIgnore` | ||
```typescript | ||
ConsoleReporter.setConsoleErrorsToIgnore([ |
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.
This is only used for situations where the console errors are acceptable and they should be more precise as the console errors only show up as a byproduct of the test (e.g. ConsoleReporter.setConsoleErrorsToIgnore([
'Blog Post with the given title exists already. Please use a different title.',
]);) <- This only shows up as an error since the test errors it. The error in this case isn't generic like status of 500 but the actual error.
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.
Done, Thanks :)
Acceptance-Tests.md
Outdated
Now, Acceptance Tests are capable of finding console errors during CUJ's and it will break the test. But in some cases where the console error can be ignored and not in high priority right now then we can ignore that with help of `consoleReporter.setConsoleErrorsToIgnore` | ||
```typescript | ||
ConsoleReporter.setConsoleErrorsToIgnore([ | ||
/Failed to load resource: the server responded with a status of 500/, |
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.
We should add another statement on how to add errors that are already existing or issues that are bugs. To ignore these, we should put them in the actual console-reporter.ts utility (should probably mention this as a file in the tree above as well). You can look in there to see how we do it but basically we put the error regex into a constant array and put a TODO to the filed issue.
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.
Done, Thanks :)
@@ -31,7 +31,8 @@ oppia/core/tests/ | |||
│ ├── puppeteer-utils.ts | |||
│ ├── show-message-utils.ts | |||
│ ├── test-constants.ts | |||
│ └── user-factory.ts | |||
│ ├── user-factory.ts | |||
│ └── console-reporter.ts |
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.
Add an explanation for this file (console-reporter.ts) inside, The directory structure is as follows... The puppeteer-testing-utilities... new bullet point.
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.
Done, Thanks
Co-authored-by: Justin Nguyen <70992422+jnvtnguyen@users.noreply.github.com>
Co-authored-by: Justin Nguyen <70992422+jnvtnguyen@users.noreply.github.com>
Co-authored-by: Justin Nguyen <70992422+jnvtnguyen@users.noreply.github.com>
…loper-docs into Acceptance-Test-doc-update
…loper-docs into Acceptance-Test-doc-update
…2134/oppia-web-developer-docs into Acceptance-Test-doc-update
@jnvtnguyen PTAL. |
1 similar comment
@jnvtnguyen PTAL. |
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.
Sorry for the delay.
'https://pencilcode.net/lib/pencilcodeembed.js - Failed to ' + | ||
'load resource: net::ERR_CERT_DATE_INVALID' | ||
), | ||
]; |
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.
Maybe add a section for CONSOLE_ERRORS_TO_FIX as well and how you should add error regexes here when you come by an error which is a bug (prompt developer to file an issue (or if already existing do not) and add a TODO). Though this will be gone once we completely remove all console error bugs.
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.
done
Acceptance-Tests.md
Outdated
|
||
We can handle these differences by including conditional statements in the `utils` file, using the `isViewportAtMobileWidth()` function to execute commands specific to mobile devices. | ||
|
||
For Eg. |
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.
For example:
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.
done
@jnvtnguyen PTAL. |
Acceptance-Tests.md
Outdated
), | ||
]; | ||
``` | ||
To handle errors that need to be fixed, you should include them directly within the `console-reporter.ts` utility. In this file, you would add the error regex to the `CONSOLE_ERRORS_TO_FIX ` array and add a TODO comment which have the details of the issue. If the error is new, also raise it in the issue section. |
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.
To handle errors that need to be fixed, you should include them directly within the `console-reporter.ts` utility. In this file, you would add the error regex to the `CONSOLE_ERRORS_TO_FIX ` array and add a TODO comment which have the details of the issue. If the error is new, also raise it in the issue section. | |
To handle errors that need to be fixed, you should include them directly within the `console-reporter.ts` utility. In this file, you would add the error regex to the `CONSOLE_ERRORS_TO_FIX ` array and add a TODO comment which points to the existing issue number. If the error doesn't have any corresponding issue, then file a new issue on our [issue tracker](https://github.com/oppia/oppia/issues). |
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.
done @jnvtnguyen PTAL.
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.
One small thing, other than that LGTM
Acceptance-Tests.md
Outdated
|
||
Similar to desktop, we also have acceptance tests for mobile to ensure responsiveness and uninterrupted user journeys on small screen devices. While the tests themselves remain largely the same for both desktop and mobile, there are some differences. For instance, large full menus on desktop may be converted to dropdowns, hamburger menus, or other shortcuts on mobile, requiring additional actions to complete the tests. | ||
|
||
### How to Write Tests for Mobile |
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.
How to write tests for mobile
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.
@jnvtnguyen Done.
@seanlip PTAL. Its approved by @jnvtnguyen. Merge this if no further review is required. |
Thanks @jnvtnguyen for the review!!! Assigning to @seanlip for final pass, Thanks! |
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 @rahat2134 (and @jnvtnguyen)! Overall this looks quite good. I just have a few small grammar/readability points, PTAL.
Acceptance-Tests.md
Outdated
@@ -126,6 +128,114 @@ make run_tests.acceptance suite="blog-editor-tests/check-blog-editor-unable-to-p | |||
|
|||
10) The test must be thoroughly tested before submitting a PR. The test can be run locally by running the following command as mentioned above or you can run the test on the CI server by pushing your code to the remote branch in your fork. The CI server will run the test and will show the result. | |||
|
|||
### Console errors logging functionality in Acceptance Tests | |||
|
|||
Acceptance Tests have the capability to detect console errors during CUJ's, potentially resulting in test failures. However, there are scenarios where certain console errors can be deemed acceptable and should not cause the test to fail. In order to ignore certain errors like these, you can use `ConsoleReporter.setConsoleErrorsToIgnore`, which takes in an array of error regexes to match the errors that can be acceptable. For instance, errors like `Blog Post with the given title exists already. Please use a different title.`, which occurs during the 'blog-editor-tests/try-to-publish-a-duplicate-blog-post-and-get-blocked' test, is ignored using the ConsoleReporter since it is an acceptable error in the context of the test. When passing acceptable errors like these to the ConsoleReporter, you should be specific and not use vague errors like `Failed to load resource...`, below is an example of this usage: |
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.
ignore certain errors --> ignore errors
errors like --> an error like
Split "Below is an example ..." at the end into its own sentence.
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.
done
Acceptance-Tests.md
Outdated
]); | ||
``` | ||
|
||
To handle errors that need to be ignored and are are not specific to any acceptance test, you should include them directly within the `console-reporter.ts` utility. In this file, you would add the error regex to the `CONSOLE_ERRORS_TO_IGNORE` array and explain with a comment why this error should be ignored. |
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.
Duplicate "are".
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.
done
Acceptance-Tests.md
Outdated
``` | ||
To handle errors that need to be fixed, you should include them directly within the `console-reporter.ts` utility. In this file, you would add the error regex to the `CONSOLE_ERRORS_TO_FIX ` array and add a TODO comment which points to the existing issue number. If the error doesn't have any corresponding issue, then file a new issue on our [issue tracker](https://github.com/oppia/oppia/issues). | ||
|
||
Note: This will be removed after the bug is resolved. |
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.
It's unclear what the note and what "this" refer to here. I suggest inlining it with the paragraph above (if you're talking about the issue) ... or clarifying if you're talking about something else.
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.
done
@seanlip PTAL. |
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, couple more things.
Acceptance-Tests.md
Outdated
To handle errors that need to be fixed, you should include them directly within the `console-reporter.ts` utility. In this file, you would add the error regex to the `CONSOLE_ERRORS_TO_FIX ` array and add a TODO comment which points to the existing issue number. If the error doesn't have any corresponding issue, then file a new issue on our [issue tracker](https://github.com/oppia/oppia/issues). | ||
|
||
Note: This will be removed after the bug is resolved. | ||
To handle errors that need to be fixed, you should include them directly within the `console-reporter.ts` utility. In this file, you would add the error regex to the `CONSOLE_ERRORS_TO_FIX ` array and add a TODO comment which points to the existing issue number(This will be removed after the bug is resolved). If the error doesn't have any corresponding issue, then file a new issue on our [issue tracker](https://github.com/oppia/oppia/issues). |
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.
... add a TODO comment which points to the existing issue number (this comment should be removed when the bug is resolved).
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.
done
Acceptance-Tests.md
Outdated
@@ -130,14 +130,16 @@ make run_tests.acceptance suite="blog-editor-tests/check-blog-editor-unable-to-p | |||
|
|||
### Console errors logging functionality in Acceptance Tests | |||
|
|||
Acceptance Tests have the capability to detect console errors during CUJ's, potentially resulting in test failures. However, there are scenarios where certain console errors can be deemed acceptable and should not cause the test to fail. In order to ignore certain errors like these, you can use `ConsoleReporter.setConsoleErrorsToIgnore`, which takes in an array of error regexes to match the errors that can be acceptable. For instance, errors like `Blog Post with the given title exists already. Please use a different title.`, which occurs during the 'blog-editor-tests/try-to-publish-a-duplicate-blog-post-and-get-blocked' test, is ignored using the ConsoleReporter since it is an acceptable error in the context of the test. When passing acceptable errors like these to the ConsoleReporter, you should be specific and not use vague errors like `Failed to load resource...`, below is an example of this usage: | |||
Acceptance Tests have the capability to detect console errors during CUJ's, potentially resulting in test failures. However, there are scenarios where certain console errors can be deemed acceptable and should not cause the test to fail. In order to ignore errors like these, you can use `ConsoleReporter.setConsoleErrorsToIgnore`, which takes in an array of error regexes to match the errors that can be acceptable. For instance, an error like `Blog Post with the given title exists already. Please use a different title.`, which occurs during the 'blog-editor-tests/try-to-publish-a-duplicate-blog-post-and-get-blocked' test, is ignored using the ConsoleReporter since it is an acceptable error in the context of the test. When passing acceptable errors like these to the ConsoleReporter, you should be specific and not use vague errors like `Failed to load resource...`. |
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.
CUJ's --> CUJs
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.
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.
LGTM, thanks @rahat2134 !
Fix #292 : This pr updates the wiki of accpetance test for mobile.