-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
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
Fix part of #9749: Migrate Preferences page #13219
Conversation
Hi @ashutoshc8101, can you complete the following:
|
Hi, @ashutoshc8101, this pull request does not have a "CHANGELOG: ..." label as mentioned in the PR pointers. Assigning @ashutoshc8101 to add the required label. PRs without this label will not be merged. If you are unsure of which label to add, please ask the reviewers for guidance. Thanks! |
Hi @ashutoshc8101, when creating WIP/Draft PRs, ensure that your commit messages are prefixed with [ci skip] or [skip ci] to prevent CI checks from running. You can learn more about it here. |
Hi @ashutoshc8101, when creating WIP/Draft PRs, ensure that your commit messages are prefixed with [ci skip] or [skip ci] to prevent CI checks from running. You can learn more about it here. |
1 similar comment
Hi @ashutoshc8101, when creating WIP/Draft PRs, ensure that your commit messages are prefixed with [ci skip] or [skip ci] to prevent CI checks from running. You can learn more about it here. |
Hi @ashutoshc8101, when creating WIP/Draft PRs, ensure that your commit messages are prefixed with [ci skip] or [skip ci] to prevent CI checks from running. You can learn more about it here. |
1 similar comment
Hi @ashutoshc8101, when creating WIP/Draft PRs, ensure that your commit messages are prefixed with [ci skip] or [skip ci] to prevent CI checks from running. You can learn more about it here. |
Hi @ashutoshc8101. Due to recent changes in the "develop" branch, this PR now has a merge conflict. Please follow this link if you need help resolving the conflict, so that the PR can be merged. Thanks! |
Hi @ashutoshc8101, the build of this PR is stale and this could result in tests failing in develop. Please update this pull request with the latest changes from develop. Thanks! |
Unassigning @darksun27 since they have already approved the PR. |
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 @ashutoshc8101! Took a pass on the codeowner files and left some questions in the e2e files. PTAL
let promiseValue = await browser.executeScript(() => { | ||
return document.getElementsByClassName( | ||
'protractor-test-base-container')[0].innerHTML; | ||
}); |
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.
Why this change?
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.
getAttribute('innerHTML')
was returning null.
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.
oh! do we know why? In general, we try to avoid "browser.executeScript" where possible because it could introduce flakiness. So this should be a last resort.
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.
The purpose of getAttribute('innerHTML')
is to get contents of this attribute.
It is not similiar to javascript innerHTML
property on html element.
This javascript property and html attribute are two different things for protractor.
It is returning null due to absence of innerHTML html attribute on base-container div element.
Earlier, there used to be a getInnerHTML
function provided by protractor.
This was removed and browser.executeScript was the replacement.
https://github.com/angular/protractor/blob/master/CHANGELOG.md#breaking-changes-3
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.
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.
Ah, I see. In that case, can you clearly document in a comment that the recommended protractor approach (as described above) is to use browser.executeScript
when innerHTML needs to be read? 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.
done
audioLanguageSelector, | ||
'audio language selector taking too long to appear.'); | ||
expect(await audioLanguageSelector.getText()) | ||
.not.toEqual(language); |
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.
nit: can we move this to line 200?
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
audioLanguageSelector, | ||
'audio language selector taking too long to appear.'); | ||
expect(await audioLanguageSelector.getText()) | ||
.toEqual(language); |
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.
nit: Please move this to the previous line.
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
await preferencesPage.selectPreferredAudioLanguage('Hungarian'); | ||
await preferencesPage.expectPreferredAudioLanguageToBe('Hungarian'); | ||
await browser.refresh(); | ||
await waitFor.pageToFullyLoad(); | ||
|
||
await preferencesPage.expectPreferredAudioLanguageToBe('Arabic'); | ||
await preferencesPage.expectPreferredAudioLanguageToBe('Hungarian'); |
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.
Why change this?
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.
protractor had trouble scrolling mat-select
. So, I tried to use a select option, which didn't require scrolling.
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.
If this is an issue with protractor and mat-select in general, can you please create an issue and assign it to the Automated QA team to investigate? We will need to find a way to fix this in case a test we write in the future uses mat-select and actually expects scrolling to be 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.
sure
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. Issue #13324
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, just restarting 2 workflows to make sure no new flakes are introduced.
Unassigning @krishnarao22 since they have already approved the PR. |
@kevintab95 @jameesjohn 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 @ashutoshc8101! LGTM for codeowner files.
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
Hey @ashutoshc8101 -- this PR might have introduced a new flake. Can you please confirm/resolve this? |
commit 1daef35 Author: Brian Rodriguez <brian@brianrodri.com> Date: Wed Jul 7 23:37:43 2021 -0400 fix lint commit c45ac4e Merge: 76360a5 aae335d Author: Brian Rodriguez <brian@brianrodri.com> Date: Wed Jul 7 23:25:52 2021 -0400 Merge branch 'fix-pylint' into index-all-activities commit aae335d Author: Brian Rodriguez <brian@brianrodri.com> Date: Wed Jul 7 23:20:26 2021 -0400 Fix part of oppia#12718: Use init-hook to fix command line calls to pylint commit 76360a5 Author: Brian Rodriguez <brian@brianrodri.com> Date: Wed Jul 7 23:03:22 2021 -0400 replace index-all-activities-job commit 1cb69a8 Author: Praneeth <praneethg2001@gmail.com> Date: Thu Jul 8 04:09:09 2021 +0530 Fixes Typescript issue in oppia-short-response-mulitple-choice-input.component and mypy issue in schema_utils.py (oppia#13335) * minor fixes * mypy fix commit e2e59bd Author: Praneeth <praneethg2001@gmail.com> Date: Thu Jul 8 00:04:55 2021 +0530 Frontend Unit Tests(3): Cover multiple component files (oppia#13303) * intial commit * minor fix commit 186435d Author: Hardik Katehara <54679643+hardikkat24@users.noreply.github.com> Date: Wed Jul 7 22:44:02 2021 +0530 Fix oppia#13239 and oppia#13171: Adding type annotations to root files and fix constants errors with mypy (oppia#13269) commit 7f5757f Author: EeshaArif <43031234+EeshaArif@users.noreply.github.com> Date: Wed Jul 7 22:13:10 2021 +0500 Fix Part of oppia#10474: Cover some files with TS strict checks - 6 (oppia#13263) commit 1577479 Author: Nikhil <57531197+Nik-09@users.noreply.github.com> Date: Wed Jul 7 21:21:14 2021 +0530 Adds schema for handlers concept_card_viewer & contributor_dashboard modules (oppia#13225) * payload_validator(created) + schema_utils(modified) for SVS architecture * payload * validate_args_schema in base + backend tests * Adds linters to verify handlers must have schema * Shows exception message in frontend when SVS validation fails. * adds schema in backend test * fix coverage * adds backend test for payload_validator into shard 1. * Changes logic for handling default_value cases. * removes dict from properties list in schema utils * fix coverage * changes codeowners * Adds backend unit tests + comments + docstrings * fix coverage * adds backend tests for coverage * fix coverage * adds backend tests for coverage * fix coverage * fix coverage * adds test into shard * backend coverage * fix coverage * structured backend test. * adds docstring * fix coverage * changes formatting for schema in MockHandlers * refactor domain objects validator * refactor handling object_dict * fixes backend tests. * validate_method -> validation_method * adds extra checks in backend test. * updates comment * adds backend tests. * Addressed review comments. * fix coverage * Addressed review comments. * fixes backend tests for linters * Addressed review comments. * updates loop into list comprehension. * addressed review comments * fix coverage * addressed review comments. * addressed review comments * addressed review comments. * fix coverage * addressed review comments * fix coverage * remove repeted code. * addressed review comments. * removes duplicate code from payload_validator * fix backend tests * schemas for contributor_dashboard+concept_card_viewer * fixes backend tests * updates logic for handling optional cases * fixes backend tests * adds backend coverage tests * refactores payload validator * adds schema type string * adds handler class name in old handlers list * fixes backend tests * fix coverage * removes dead code. * makes backend code typed. * adds new handler class in old handler * addressed review comments * fix coverage * fix coverage * updates logic for handling normalized value * changes string -> basestring * updates schema. * updates schemas * addressed review comments * updates by adding schema key 'schema' * adds validator for language code. * fix coverage * adds comment and updates docstring. * udates comment. * updates comment * updates comment * updates comment * updates schemas * changes docstring. * updates logic to handle csrf and source * fixes backend tests * updates logic to raise 400 * updates logic to handle source * updates logic * removes if-block in dispatch method * adds previous if-else logic to handle source * updates if into elif * adds comment in csrf_token * updates logic and adds tests for backend coverage. * fixes coverage * updates comment * uses sanitize url instead of regex. * fix coverage * fix coverage. * uses regex to check source url. commit f9a5144 Author: Ashutosh Chauhan <ashutoshc8101@gmail.com> Date: Wed Jul 7 16:10:51 2021 +0530 Fix part of oppia#9749: Migrate Preferences page (oppia#13219) commit 6b1f473 Author: Brian Rodriguez <brian@brianrodri.com> Date: Wed Jul 7 00:31:34 2021 -0400 Fix part of oppia#11475: Delete all references to continuous jobs (oppia#13313) * Removed notification page * Fixed merge conflict file * Fixed ts error * Removed notification from accessiblity test * Fixed JS lint issue * Changed the shortcut for preferenceURL * additional nits * Fixed ts error * reverting removal of userbackend service * Added back the changes * Delete FeedbackAnalyticsAggregator * fix unused code * add PR to note * small nit * address comments * initial attempt * initial attempt * fix ratings test * delete file * reduce untested lines * fix lint * remove frontend references to continuous jobs * remove backend references to continuous jobs * remove checks for events * fix lint * delete QUEUE_NAME_EVENTS * delete events from queue.yaml * fix bugs * fix queue.yaml tests * fix coverage * fix lint Co-authored-by: Aasif Faizal <aasif.faizal@gmail.com> commit 4c64eee Author: Rijuta Singh <68547101+Rijuta-s@users.noreply.github.com> Date: Wed Jul 7 06:47:32 2021 +0530 Milestone 1.5 : Adds blog admin page frontend. (oppia#13300) * Adding blog admin page * adding blog admin page * fixing lint * minor fixes * [ci skip] adding spec file * adding spec file * applying suggestions * applying suggestions * Applting lint suggestions * applying suggestions * fixing lint issues * sdcsd * applying suggestions * Revert adding root component files This reverts commit 6bd0d76. * applying suggestions * minor fix commit a2f9d14 Author: Aishwary Saxena <aishwary.saxena.min19@itbhu.ac.in> Date: Tue Jul 6 18:09:15 2021 +0530 Unit test for questions-list.component.ts (oppia#13287) * Converted AJS Directive to AJS Component * Added tests * Added tests, completed 100% * Fixed lint checks * fix karma conf * Addressed review commit 79e6d10 Author: Aishwary Saxena <aishwary.saxena.min19@itbhu.ac.in> Date: Tue Jul 6 14:49:00 2021 +0530 Unit test for exploration creation service (oppia#13176) * Added tests * Lint err * Added callback for dataFilter * Removed file name from NOT_FULLY_COVERED_FILENAMES * Added expectations * Nit * Nit * Addressed review * Removed fdescribe * Addressed review * lint * Removed mock * Fix * removed fdescribe commit e0a39a2 Author: Farees Hussain <fareezzhussain@gmail.com> Date: Tue Jul 6 13:14:13 2021 +0530 Fix oppia#13172: Controller to initialize android specific structures (Milestone 1.2) (oppia#13174) * controller to initiate android specific structures * fix linters * using variable for exploration id * fix linters * upload thumbnails * added tests * fix linters * remove csrf token * added to shards * refactoring and testcases * fix CI checks * fix CI * Addressed requested changes * rename test * timeout 30 with comment * fix linter * remove post_req * added render_json * NIT * changed comment * whitespace * requested changes * more docs * NIT * docstring update * addressed suggestions * replaced 500 with 400 * update docstring * updated docstring commit d12bfc1 Author: Kevin Thomas <kevintab@tutanota.com> Date: Tue Jul 6 12:08:19 2021 +0530 Fix typescript check failure (oppia#13321) commit c037e83 Author: Kevin Thomas <kevintab@tutanota.com> Date: Tue Jul 6 11:33:46 2021 +0530 Modify translation submission validation to check for custom tags (oppia#13309) commit 220c03a Author: Radesh <54100702+Radesh-kumar@users.noreply.github.com> Date: Tue Jul 6 10:42:21 2021 +0530 Added unit tests for admin roles tab (oppia#13206) commit 56a9e43 Author: Kevin Thomas <kevintab@tutanota.com> Date: Tue Jul 6 09:57:02 2021 +0530 Add Marathi (मराठी) and Ganda (Luganda) to supported languages (oppia#13315) commit c82945e Author: AdityaDubey0 <55298925+AdityaDubey0@users.noreply.github.com> Date: Tue Jul 6 03:53:03 2021 +0530 Migrate lint check to disallow innerHTML property. (oppia#13301) commit 901bc8b Author: Hitesh Tomar <44300735+lkbhitesh07@users.noreply.github.com> Date: Tue Jul 6 02:33:38 2021 +0530 warning text is within the box (oppia#13296) commit 90d0152 Author: AdityaDubey0 <55298925+AdityaDubey0@users.noreply.github.com> Date: Tue Jul 6 01:45:41 2021 +0530 Disallow $parent and $broadcast angularJs property (oppia#13291)
Tested locally on develop, 5/5 passing. |
Yep, this was not due the changes in my PR as the changes were in unrelated files. Can you run the tests on local fork? |
Yup, I still get this error sometimes. Apart from this, I am getting one more frontend coverage check error, which I guess is related to this PR. PS: The failing test in @aishwary023's comment is related to the same file which is shown not covered to me. |
@aishwary023 @atpug22 I am looking into it. Thanks for confirming the flake. |
Fixed in #13419. Thanks |
Overview
Essential Checklist
Proof that changes are correct
preferences-page-2021-07-02_17.49.11.mp4
PR Pointers