MCKIN-12920 Feedback close button ARIA - #264
Conversation
|
@murad-hubib Please fix the tests as well. |
|
@xitij2000 These test failures are related to this pr? I have a birds eye view over and it seems tests are not related to this particular change. |
They are related to a previous MCKIN PR that was merged, and as such need to be fixed here since there weren't in the context of the previous PR. |
|
@xitij2000 We have fixed all related test cases. No test case failing at our local side now. There are two test cases still failing on circleci intermittently. Can you please have a look on either these are flaky test or what else going wrong. |
xitij2000
left a comment
xitij2000
left a comment
There was a problem hiding this comment.
I do not think removing tests is an acceptable solution when the actual code hasn't changed that much. Please fix the existing tests so they work with the new structure.
Additionally, using sleep slows down tests and can make tests flaky. Please add explicit waits for elements on the page instead of sleep.
|
|
||
| # Now reload the page... | ||
| mentoring = self.reload_student_view() | ||
| time.sleep(3) |
There was a problem hiding this comment.
This is not an acceptable solution, please use a selenium wait for the UI to update.
There was a problem hiding this comment.
removed sleep here and all over changes
| item_feedback_popup = choice_wrapper.find_element_by_css_selector(".choice-tips") | ||
|
|
||
| self.assertFalse(item_feedback_popup.is_displayed()) | ||
| self.assertEqual(item_feedback_popup.text, "") |
There was a problem hiding this comment.
Why are these tests being removed when the UI elements have not been removed?
There was a problem hiding this comment.
Hi @xitij2000, thanks for reviewing. As per the changes on this PR #256, these scenarios become obsolete. so I tried to update accordingly. Actually, I had updated this test case in the past as we are facing empty pop up without performing submission after rendering a page. By the recent changes of #256, we wouldn't face these scenarios.
There was a problem hiding this comment.
I don't see how these are obsolete. The span is now a button in a number of places, but the actual functionality being tested here is intact. This is existing tests instead of adapting them.
There was a problem hiding this comment.
Yeah, you are right on it's changed into a button but I'm saying in the context of CSS changes that are applied to this button as it will not be displayed until it is selected during the form submission. By the way, I've restored this test with a minor change. Thanks
|
@xitij2000 I have addressed the request changes. One flaky test is getting failed for py35-integration intermittently but not failing at our local. Can you please review the changes and rerun the tests on Circleci if we are good. Thanks |
xitij2000
left a comment
xitij2000
left a comment
There was a problem hiding this comment.
The rest of the PR is good to go. If the removed tests are restored this is good to go.
| item_feedback_popup = choice_wrapper.find_element_by_css_selector(".choice-tips") | ||
|
|
||
| self.assertFalse(item_feedback_popup.is_displayed()) | ||
| self.assertEqual(item_feedback_popup.text, "") |
There was a problem hiding this comment.
I don't see how these are obsolete. The span is now a button in a number of places, but the actual functionality being tested here is intact. This is existing tests instead of adapting them.
|
Hi @xitij2000, a kind reminder for a review as I have addressed your last request. Thanks |
xitij2000
left a comment
xitij2000
left a comment
There was a problem hiding this comment.
👍 Good to go once the version is updated.
- I read through the code
- I checked for accessibility issues
| # Constants ######################################################### | ||
|
|
||
| VERSION = '3.4.9' | ||
| VERSION = '3.5.0' |
There was a problem hiding this comment.
nit: change this to 3.4.10, this is a very minor change.
|
@xitij2000 I have updated the version and say thank you for taking your time. |
|
@mudassir-hafeez Thanks! I will merge and tag this once tests pass. |
4 participants
Ticket: https://edx-wiki.atlassian.net/browse/MCKIN-12920
@xitij2000 Please review and merge