Skip to content
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

refactor: lightbox screen #15996

Merged
merged 7 commits into from
May 24, 2023
Merged

refactor: lightbox screen #15996

merged 7 commits into from
May 24, 2023

Conversation

OmarBasem
Copy link
Contributor

This PR fixes functional components usage in lightbox screen and does some refactoring.

@OmarBasem OmarBasem self-assigned this May 23, 2023
Comment on lines +1 to +2
(ns status-im2.contexts.chat.lightbox.utils
(:require
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file is not a new code. These methods where initially in the view file.

Copy link
Contributor

Choose a reason for hiding this comment

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

I would have some comments for this file, but I'm not sure if it's worth sharing. I'm assuming the idea of this PR is to mostly move the code without many changes, is that correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes correct. But feel free to share your comments (tho I am sure you reviewed this code before :) )

Copy link
Contributor

Choose a reason for hiding this comment

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

I did, and you know @OmarBasem, code reviews are funny, it's like coding where the solution usually doesn't come out easily, requires trial and error. Similarly, in code reviews, as I familiarize myself with the new code (PR after PR being merged), the fog gradually dissipates.

Well, enough of comparisons!

@status-im-auto
Copy link
Member

status-im-auto commented May 23, 2023

Jenkins Builds

Click to see older builds (12)
Commit #️⃣ Finished (UTC) Duration Platform Result
61c0d3a #1 2023-05-23 13:57:33 ~3 min tests 📄log
✔️ 61c0d3a #1 2023-05-23 14:00:19 ~6 min android-e2e 🤖apk 📲
✔️ 61c0d3a #1 2023-05-23 14:00:28 ~6 min android 🤖apk 📲
✔️ 7b3c33f #2 2023-05-23 14:08:10 ~6 min android-e2e 🤖apk 📲
✔️ 7b3c33f #2 2023-05-23 14:08:42 ~7 min tests 📄log
✔️ 7b3c33f #2 2023-05-23 14:09:42 ~8 min ios 📱ipa 📲
✔️ 7b3c33f #2 2023-05-23 14:12:25 ~10 min android 🤖apk 📲
b88deae #3 2023-05-24 06:39:01 ~2 min ios 📄log
d072a48 #4 2023-05-24 06:41:56 ~1 min ios 📄log
✔️ d072a48 #4 2023-05-24 06:45:44 ~5 min android-e2e 🤖apk 📲
✔️ d072a48 #4 2023-05-24 06:46:10 ~6 min tests 📄log
✔️ d072a48 #4 2023-05-24 06:46:19 ~6 min android 🤖apk 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
bb2a00a #5 2023-05-24 14:16:58 ~3 min tests 📄log
✔️ bb2a00a #5 2023-05-24 14:19:10 ~5 min android 🤖apk 📲
✔️ bb2a00a #5 2023-05-24 14:19:29 ~5 min android-e2e 🤖apk 📲
✔️ bb2a00a #5 2023-05-24 14:19:29 ~6 min ios 📱ipa 📲
✔️ e3d3bd0 #6 2023-05-24 18:18:12 ~5 min android 🤖apk 📲
✔️ e3d3bd0 #6 2023-05-24 18:18:35 ~6 min ios 📱ipa 📲
✔️ e3d3bd0 #6 2023-05-24 18:18:45 ~6 min android-e2e 🤖apk 📲
✔️ e3d3bd0 #6 2023-05-24 18:20:32 ~8 min tests 📄log

@status-im-auto
Copy link
Member

57% of end-end tests have passed

Total executed tests: 28
Failed tests: 12
Passed tests: 16
Not executed tests: 5
IDs of not executed tests: 702846,702742,703133,702850,702851 
IDs of failed tests: 702958,702732,702782,702786,702894,702783,702807,702845,702957,702808,703086,702838 

Not executed tests (5)

Click to expand
  • Rerun not executed tests
  • Failed tests (12)

    Click to expand
  • Rerun failed tests

  • Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_emoji_send_reply_and_open_link, id: 702782

    Device 1: Tap on found: SendMessageButton
    Device 2: Find `Button` by `xpath`: `//*[starts-with(@text,'http://status.im')]`

    critical/chats/test_1_1_public_chats.py:930: in test_1_1_chat_emoji_send_reply_and_open_link
        self.chat_2.element_starts_with_text(url_message, 'button').click_inside_element_by_coordinate(0.2, 0.5)
    ../views/base_element.py:328: in click_inside_element_by_coordinate
        location, size = self.get_element_coordinates()
    ../views/base_element.py:266: in get_element_coordinates
        element = self.find_element()
    ../views/base_element.py:80: in find_element
        raise NoSuchElementException(
     Device 2: Button by xpath: `//*[starts-with(@text,'http://status.im')]` is not found on the screen
    



    Device sessions

    2. test_1_1_chat_is_shown_message_sent_delivered_from_offline, id: 702783

    # STEP: Device1 goes back online and checks that 1-1 chat will be fetched
    Device 1: Looking for a message by text: test message

    critical/chats/test_1_1_public_chats.py:1230: in test_1_1_chat_is_shown_message_sent_delivered_from_offline
        self.errors.verify_no_errors()
    base_test_case.py:184: in verify_no_errors
        pytest.fail('\n '.join([self.errors.pop(0) for _ in range(len(self.errors))]))
     Message status was not delivered after back up online, it is "Sending"!
    



    Device sessions

    Class TestGroupChatMultipleDeviceMergedNewUI:

    1. test_group_chat_pin_messages, id: 702732

    Test setup failed: critical/chats/test_group_chat.py:198: in prepare_devices
        self.chats[0] = self.homes[0].create_group_chat(user_names_to_add=[self.usernames[1], self.usernames[2]]</b>,
    ../views/home_view.py:358: in create_group_chat
        [chat.get_username_checkbox(user_name).click() for user_name in user_names_to_add]
    ../views/home_view.py:358: in <listcomp>
        [chat.get_username_checkbox(user_name).click() for user_name in user_names_to_add]
    ../views/chat_view.py:326: in click
        self.scroll_to_element(direction='up', depth=20).click()
    ../views/base_element.py:177: in scroll_to_element
        raise NoSuchElementException(
     Device 1: UsernameCheckbox by xpath: `//*[@text='member_1']/..//*[@content-desc='checkbox-off']` is not found on the screen
    



    2. test_group_chat_join_send_text_messages_push, id: 702807

    Device 1: Find UsernameCheckbox by xpath: //*[@text='member_1']/..//*[@content-desc='checkbox-off']
    Device 1: Find UsernameCheckbox by xpath: //*[@text='member_1']/..//*[@content-desc='checkbox-off']

    Test setup failed: critical/chats/test_group_chat.py:198: in prepare_devices
        self.chats[0] = self.homes[0].create_group_chat(user_names_to_add=[self.usernames[1], self.usernames[2]]</b>,
    ../views/home_view.py:358: in create_group_chat
        [chat.get_username_checkbox(user_name).click() for user_name in user_names_to_add]
    ../views/home_view.py:358: in <listcomp>
        [chat.get_username_checkbox(user_name).click() for user_name in user_names_to_add]
    ../views/chat_view.py:326: in click
        self.scroll_to_element(direction='up', depth=20).click()
    ../views/base_element.py:177: in scroll_to_element
        raise NoSuchElementException(
     Device 1: UsernameCheckbox by xpath: `//*[@text='member_1']/..//*[@content-desc='checkbox-off']` is not found on the screen
    



    Device sessions

    3. test_group_chat_offline_pn, id: 702808

    Test setup failed: critical/chats/test_group_chat.py:198: in prepare_devices
        self.chats[0] = self.homes[0].create_group_chat(user_names_to_add=[self.usernames[1], self.usernames[2]]</b>,
    ../views/home_view.py:358: in create_group_chat
        [chat.get_username_checkbox(user_name).click() for user_name in user_names_to_add]
    ../views/home_view.py:358: in <listcomp>
        [chat.get_username_checkbox(user_name).click() for user_name in user_names_to_add]
    ../views/chat_view.py:326: in click
        self.scroll_to_element(direction='up', depth=20).click()
    ../views/base_element.py:177: in scroll_to_element
        raise NoSuchElementException(
     Device 1: UsernameCheckbox by xpath: `//*[@text='member_1']/..//*[@content-desc='checkbox-off']` is not found on the screen
    



    Class TestCommunityMultipleDeviceMerged:

    1. test_community_mentions_push_notification, id: 702786

    Test setup failed: base_test_case.py:356: in setup_method
        driver.execute_script("sauce:context=Started %s" % method.__name__)
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webdriver.py:634: in execute_script
        return self.execute(command, {
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webdriver.py:321: in execute
        self.error_handler.check_response(response)
    /home/jenkins/.local/lib/python3.10/site-packages/appium/webdriver/errorhandler.py:31: in check_response
        raise wde
    /home/jenkins/.local/lib/python3.10/site-packages/appium/webdriver/errorhandler.py:26: in check_response
        super().check_response(response)
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/errorhandler.py:208: in check_response
        raise exception_class(value)
     The test with session id ccc6b7af8cf64535b127ab1391b274b7 has already finished, and can't receive further commands.
    E   For help, please check https://docs.saucelabs.com/dev/error-messages
    



    2. test_community_contact_block_unblock_offline, id: 702894

    Device 1: Looking for a message by text: I should not be in chat
    Device 1: Looking for a message by text: Message from blocked user

    critical/test_public_chat_browsing.py:711: in test_community_contact_block_unblock_offline
        if self.chat_1.chat_element_by_text(message).is_element_displayed(30):
    ../views/base_element.py:193: in is_element_displayed
        return self.wait_for_visibility_of_element(sec, ignored_exceptions=ignored_exceptions)
    ../views/base_element.py:133: in wait_for_visibility_of_element
        .until(expected_conditions.visibility_of_element_located((self.by, self.locator)))
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/support/wait.py:71: in until
        value = method(self._driver)
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/support/expected_conditions.py:128: in __call__
        return _element_if_visible(_find_element(driver, self.locator))
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/support/expected_conditions.py:415: in _find_element
        raise e
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/support/expected_conditions.py:411: in _find_element
        return driver.find_element(*by)
    /home/jenkins/.local/lib/python3.10/site-packages/appium/webdriver/webdriver.py:414: in find_element
        return self.execute(RemoteCommand.FIND_ELEMENT, {'using': by, 'value': value})['value']
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webdriver.py:321: in execute
        self.error_handler.check_response(response)
    /home/jenkins/.local/lib/python3.10/site-packages/appium/webdriver/errorhandler.py:31: in check_response
        raise wde
    /home/jenkins/.local/lib/python3.10/site-packages/appium/webdriver/errorhandler.py:26: in check_response
        super().check_response(response)
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/errorhandler.py:242: in check_response
        raise exception_class(message, screen, stacktrace)
     An unknown server-side error occurred while processing the command. Original error: Could not proxy command to the remote server. Original error: socket hang up
    



    Device sessions

    3. test_community_leave, id: 702845

    Test setup failed: base_test_case.py:356: in setup_method
        driver.execute_script("sauce:context=Started %s" % method.__name__)
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webdriver.py:634: in execute_script
        return self.execute(command, {
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webdriver.py:321: in execute
        self.error_handler.check_response(response)
    /home/jenkins/.local/lib/python3.10/site-packages/appium/webdriver/errorhandler.py:31: in check_response
        raise wde
    /home/jenkins/.local/lib/python3.10/site-packages/appium/webdriver/errorhandler.py:26: in check_response
        super().check_response(response)
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/errorhandler.py:208: in check_response
        raise exception_class(value)
     The test with session id ccc6b7af8cf64535b127ab1391b274b7 has already finished, and can't receive further commands.
    E   For help, please check https://docs.saucelabs.com/dev/error-messages
    



    4. test_community_mark_all_messages_as_read, id: 703086

    Test setup failed: base_test_case.py:356: in setup_method
        driver.execute_script("sauce:context=Started %s" % method.__name__)
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webdriver.py:634: in execute_script
        return self.execute(command, {
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webdriver.py:321: in execute
        self.error_handler.check_response(response)
    /home/jenkins/.local/lib/python3.10/site-packages/appium/webdriver/errorhandler.py:31: in check_response
        raise wde
    /home/jenkins/.local/lib/python3.10/site-packages/appium/webdriver/errorhandler.py:26: in check_response
        super().check_response(response)
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/errorhandler.py:208: in check_response
        raise exception_class(value)
     The test with session id ccc6b7af8cf64535b127ab1391b274b7 has already finished, and can't receive further commands.
    E   For help, please check https://docs.saucelabs.com/dev/error-messages
    



    5. test_community_message_send_check_timestamps_sender_username, id: 702838

    Device 2: Verifying that 'hello' is under today
    Device 2: Looking for a message by text: hello

    critical/test_public_chat_browsing.py:456: in test_community_message_send_check_timestamps_sender_username
        channel.verify_message_is_under_today_text(message, self.errors)
    ../views/chat_view.py:944: in verify_message_is_under_today_text
        message_element.wait_for_visibility_of_element()
    ../views/base_element.py:135: in wait_for_visibility_of_element
        raise TimeoutException(
     Device 2: ChatElementByText by xpath:`//*[starts-with(@text,'hello')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']` is not found on the screen after wait_for_visibility_of_element 
    

    [[blocked by 14797]]

    Device sessions

    Class TestActivityMultipleDevicePR:

    1. test_activity_center_admin_notification_accept_swipe, id: 702958

    Test setup failed: base_test_case.py:356: in setup_method
        driver.execute_script("sauce:context=Started %s" % method.__name__)
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webdriver.py:634: in execute_script
        return self.execute(command, {
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webdriver.py:321: in execute
        self.error_handler.check_response(response)
    /home/jenkins/.local/lib/python3.10/site-packages/appium/webdriver/errorhandler.py:31: in check_response
        raise wde
    /home/jenkins/.local/lib/python3.10/site-packages/appium/webdriver/errorhandler.py:26: in check_response
        super().check_response(response)
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/errorhandler.py:208: in check_response
        raise exception_class(value)
     ERROR The test with session id 46d8e44a75ce474b9e8f42826f076be0 has already finished, and can't receive further commands.
    E   You can learn more at https://app.eu-central-1.saucelabs.com/tests/46d8e44a75ce474b9e8f42826f076be0
    E   For help, please check https://docs.saucelabs.com/dev/error-messages
    



    2. test_activity_center_mentions, id: 702957

    Test setup failed: base_test_case.py:356: in setup_method
        driver.execute_script("sauce:context=Started %s" % method.__name__)
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webdriver.py:634: in execute_script
        return self.execute(command, {
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/webdriver.py:321: in execute
        self.error_handler.check_response(response)
    /home/jenkins/.local/lib/python3.10/site-packages/appium/webdriver/errorhandler.py:31: in check_response
        raise wde
    /home/jenkins/.local/lib/python3.10/site-packages/appium/webdriver/errorhandler.py:26: in check_response
        super().check_response(response)
    /home/jenkins/.local/lib/python3.10/site-packages/selenium/webdriver/remote/errorhandler.py:208: in check_response
        raise exception_class(value)
     ERROR The test with session id 46d8e44a75ce474b9e8f42826f076be0 has already finished, and can't receive further commands.
    E   You can learn more at https://app.eu-central-1.saucelabs.com/tests/46d8e44a75ce474b9e8f42826f076be0
    E   For help, please check https://docs.saucelabs.com/dev/error-messages
    



    Passed tests (16)

    Click to expand

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_text_message_delete_push_disappear, id: 702733
    Device sessions

    2. test_1_1_chat_edit_message, id: 702855
    Device sessions

    3. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
    Device sessions

    4. test_1_1_chat_message_reaction, id: 702730
    Device sessions

    5. test_1_1_chat_pin_messages, id: 702731
    Device sessions

    6. test_1_1_chat_push_emoji, id: 702813
    Device sessions

    7. test_1_1_chat_delete_via_long_press_relogin, id: 702784
    Device sessions

    Class TestCommunityMultipleDeviceMerged:

    1. test_community_emoji_send_copy_paste_reply, id: 702840
    Device sessions

    2. test_community_several_images_send_reply, id: 703194
    Device sessions

    3. test_community_one_image_send_reply, id: 702859
    Device sessions

    4. test_community_links_with_previews_github_youtube_twitter_gif_send_enable, id: 702844
    Device sessions

    5. test_community_unread_messages_badge, id: 702841
    Device sessions

    6. test_community_message_delete, id: 702839
    Device sessions

    7. test_community_message_edit, id: 702843
    Device sessions

    Class TestActivityMultipleDevicePR:

    1. test_activity_center_reply_read_unread_delete_filter_swipe, id: 702947
    Device sessions

    2. test_navigation_jump_to, id: 702936
    Device sessions

    src/status_im2/contexts/chat/lightbox/view.cljs Outdated Show resolved Hide resolved
    src/status_im2/contexts/chat/lightbox/view.cljs Outdated Show resolved Hide resolved
    src/status_im2/contexts/chat/lightbox/bottom_view.cljs Outdated Show resolved Hide resolved
    Comment on lines +1 to +2
    (ns status-im2.contexts.chat.lightbox.utils
    (:require
    Copy link
    Contributor

    Choose a reason for hiding this comment

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

    I would have some comments for this file, but I'm not sure if it's worth sharing. I'm assuming the idea of this PR is to mostly move the code without many changes, is that correct?

    Copy link
    Contributor

    @ulisesmac ulisesmac left a comment

    Choose a reason for hiding this comment

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

    Thanks for the PR! :)

    I left some comments

    src/status_im2/contexts/chat/lightbox/bottom_view.cljs Outdated Show resolved Hide resolved
    src/status_im2/contexts/chat/lightbox/constants.cljs Outdated Show resolved Hide resolved
    src/status_im2/contexts/chat/lightbox/view.cljs Outdated Show resolved Hide resolved
    src/status_im2/contexts/chat/lightbox/view.cljs Outdated Show resolved Hide resolved
    src/status_im2/contexts/chat/lightbox/view.cljs Outdated Show resolved Hide resolved
    Copy link
    Contributor

    @ulisesmac ulisesmac left a comment

    Choose a reason for hiding this comment

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

    Thanks Omar!
    Looks good

    @OmarBasem OmarBasem merged commit 0cc631c into develop May 24, 2023
    @OmarBasem OmarBasem deleted the lightbox-refactor branch May 24, 2023 18:32
    @Parveshdhull
    Copy link
    Member

    Parveshdhull commented May 24, 2023

    Hi @OmarBasem,
    Thank you very much for your really awesome work.
    I am just curious why we are skipping manual testing for most of the PRs.
    And looks like even half of e2e failed in this PR, are they somehow not relevant?

    @OmarBasem
    Copy link
    Contributor Author

    OmarBasem commented May 24, 2023

    Hi @Parveshdhull this PR is just refactoring. E2e failed tests don't seem related to this PR. E2e tests a lot of times just fail, and then they pass on rerunning them.

    I ask for QA whenever needed.

    @du82
    Copy link
    Contributor

    du82 commented May 25, 2023

    Hi @OmarBasem, Thank you very much for your really awesome work. I am just curious why we are skipping manual testing for most of the PRs. And looks like even half of e2e failed in this PR, are they somehow not relevant?

    the tests are failing because of the icon change PR I believe. Ever since that one the app just crashes

    rahulpsingh pushed a commit that referenced this pull request May 26, 2023
    * refactor: lightbox screen
    @churik
    Copy link
    Member

    churik commented May 26, 2023

    @OmarBasem please, next time if you're not sure about the reason why e2e failed, can you please ask QA for help?
    I know they are fragile, but it is not a reason to skip this step completely.

    @OmarBasem
    Copy link
    Contributor Author

    Sure thing!

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    None yet
    Projects
    No open projects
    Archived in project
    Development

    Successfully merging this pull request may close these issues.

    7 participants