Hi Yosry
On Tue, Aug 13, 2019 at 6:33 PM Yosry Muhammad <yosry...@gmail.com> wrote:
> Any more suggestions or comments on the patch?
>
I am reviewing it.
>
> On Tue, Aug 13, 2019 at 2:55 PM Aditya Toshniwal <
> aditya.toshni...@enterprisedb.com> wrote:
>
>>
>>
>> On Tue, Aug 13, 2019 at 5:35 PM Yosry Muhammad <yosry...@gmail.com>
>> wrote:
>>
>>> Please find attached an updated patch with a small modification to the
>>> feature test. This makes sure the first history element is selected and
>>> focused before trying to move through entries using the down arrow key.
>>> When the test fails, is it always the same error you sent before ?
>>>
>> Seems to be working now. And yes, I got the same error every time it
>> had failed.
>>
>>>
>>> On Tue, Aug 13, 2019 at 1:51 PM Yosry Muhammad <yosry...@gmail.com>
>>> wrote:
>>>
>>>> Is it always the same error when it fails?
>>>>
>>>> On Tue, Aug 13, 2019 at 1:50 PM Aditya Toshniwal <
>>>> aditya.toshni...@enterprisedb.com> wrote:
>>>>
>>>>> The test cases fails intermittently. It passes sometimes.
>>>>>
>>>>> On Tue, Aug 13, 2019 at 5:15 PM Yosry Muhammad <yosry...@gmail.com>
>>>>> wrote:
>>>>>
>>>>>> Hi Aditya,
>>>>>>
>>>>>> The test passes on my computer, is there anything I could try to
>>>>>> reproduced the failure?
>>>>>>
>>>>>> By looking at the error, I suspect clicking the down arrow key on
>>>>>> your machine did not move to the next history entry during the test. Does
>>>>>> clicking the down arrow normally go to the next history entry on your
>>>>>> machine?
>>>>>>
>>>>>> On Tue, Aug 13, 2019 at 1:26 PM Aditya Toshniwal <
>>>>>> aditya.toshni...@enterprisedb.com> wrote:
>>>>>>
>>>>>>> Hi Yosry,
>>>>>>>
>>>>>>> Everything looks fine to me. Except intermediate feature test
>>>>>>> failure. May be @committers can try on their machine.
>>>>>>>
>>>>>>> ======================================================================
>>>>>>> FAIL: runTest
>>>>>>> (pgadmin.feature_tests.query_tool_journey_test.QueryToolJourneyTest)
>>>>>>> Tests the path through the query tool
>>>>>>>
>>>>>>> ----------------------------------------------------------------------
>>>>>>> Traceback (most recent call last):
>>>>>>> File
>>>>>>> "/Users/adityatoshniwal/projects/pgadmin4/web/pgadmin/feature_tests/query_tool_journey_test.py",
>>>>>>> line 78, in runTest
>>>>>>> self._test_query_sources_and_generated_queries()
>>>>>>> File
>>>>>>> "/Users/adityatoshniwal/projects/pgadmin4/web/pgadmin/feature_tests/query_tool_journey_test.py",
>>>>>>> line 186, in _test_query_sources_and_generated_queries
>>>>>>> self._test_history_query_sources()
>>>>>>> File
>>>>>>> "/Users/adityatoshniwal/projects/pgadmin4/web/pgadmin/feature_tests/query_tool_journey_test.py",
>>>>>>> line 215, in _test_history_query_sources
>>>>>>> history_entries_icons)
>>>>>>> File
>>>>>>> "/Users/adityatoshniwal/projects/pgadmin4/web/pgadmin/feature_tests/query_tool_journey_test.py",
>>>>>>> line 287, in _check_history_queries_and_icons
>>>>>>> self.assertIn(query, query_history_selected_item.text)
>>>>>>> AssertionError: "UPDATE public.test_editable_table2293 SET
>>>>>>> normal_column = '10'::numeric WHERE pk_column = '1';" not found in
>>>>>>> 'COMMIT;\n15:00:40'
>>>>>>>
>>>>>>> On Tue, Aug 13, 2019 at 5:03 AM Yosry Muhammad <yosry...@gmail.com>
>>>>>>> wrote:
>>>>>>>
>>>>>>>> The test failed after merging with master. A previously written
>>>>>>>> test needed to be updated after a previous commit.
>>>>>>>>
>>>>>>>> Please find an updated patch attached with the fix.
>>>>>>>>
>>>>>>>> On Mon, Aug 12, 2019 at 1:34 PM Aditya Toshniwal <
>>>>>>>> aditya.toshni...@enterprisedb.com> wrote:
>>>>>>>>
>>>>>>>>> Hi Yosry,
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Mon, Aug 12, 2019 at 2:00 PM Yosry Muhammad <yosry...@gmail.com>
>>>>>>>>> wrote:
>>>>>>>>>
>>>>>>>>>> Hi Aditya,
>>>>>>>>>>
>>>>>>>>>> On Mon, Aug 12, 2019 at 9:04 AM Aditya Toshniwal <
>>>>>>>>>> aditya.toshni...@enterprisedb.com> wrote:
>>>>>>>>>>
>>>>>>>>>>> Hi Yosry,
>>>>>>>>>>>
>>>>>>>>>>> Nice work there. It seems to be working fine except a few
>>>>>>>>>>> suggestions:
>>>>>>>>>>> 1) Fix pep8 issues
>>>>>>>>>>> 2) DOM Statements like below can be avoided and html can be
>>>>>>>>>>> added directly to main template of $el instead of adding extra
>>>>>>>>>>> operations
>>>>>>>>>>> of find, prepend and append. Plus, it makes it difficult to
>>>>>>>>>>> understand what
>>>>>>>>>>> will the DOM look like.
>>>>>>>>>>> this.$el.find('.query').prepend('<i id="query_source_icon"
>>>>>>>>>>> class="query-history-icon sql-icon-lg"></i>');
>>>>>>>>>>> $container.append($toggleEl).append(self.$el);
>>>>>>>>>>> 3) Change the below to use class d-none with
>>>>>>>>>>> toggleClass('d-none') for consistency across.
>>>>>>>>>>>
>>>>>>>>>>> this.$el.find('.pgadmin-query-history-entry').each(function() {
>>>>>>>>>>> $(this).toggle();
>>>>>>>>>>> });
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Please find an updated patch attached with the above issues
>>>>>>>>>> fixed. The pep8 issue was in the test, I didn't re-check pep8 after
>>>>>>>>>> writing
>>>>>>>>>> the test - my bad.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>>> 4) I may be wrong, but I'm seeing the flash icon for view/edit
>>>>>>>>>>> data queries and view table icon for query tool queries. Looks like
>>>>>>>>>>> they
>>>>>>>>>>> are swapped.
>>>>>>>>>>> [image: Screenshot 2019-08-12 at 12.15.18.png]
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>> They seem to be in the right place for me, would you mind
>>>>>>>>>> rechecking?
>>>>>>>>>>
>>>>>>>>> Now there are showing fine.
>>>>>>>>> However, the feature test case is failing for me. I tried 2 times:
>>>>>>>>> =============Running the test cases for 'PostgreSQL
>>>>>>>>> 11'=============
>>>>>>>>> runTest
>>>>>>>>> (pgadmin.feature_tests.query_tool_journey_test.QueryToolJourneyTest)
>>>>>>>>> Tests the path through the query tool ... Copy rows... OK.
>>>>>>>>> Copy columns... OK.
>>>>>>>>> History tab... OK.
>>>>>>>>> Updatable resultsets...FAIL
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ======================================================================
>>>>>>>>> FAIL: runTest
>>>>>>>>> (pgadmin.feature_tests.query_tool_journey_test.QueryToolJourneyTest)
>>>>>>>>> Tests the path through the query tool
>>>>>>>>>
>>>>>>>>> ----------------------------------------------------------------------
>>>>>>>>> Traceback (most recent call last):
>>>>>>>>> File
>>>>>>>>> "/Users/adityatoshniwal/projects/pgadmin4/web/pgadmin/feature_tests/query_tool_journey_test.py",
>>>>>>>>> line 79, in runTest
>>>>>>>>> self._test_updatable_resultset()
>>>>>>>>> File
>>>>>>>>> "/Users/adityatoshniwal/projects/pgadmin4/web/pgadmin/feature_tests/query_tool_journey_test.py",
>>>>>>>>> line 240, in _test_updatable_resultset
>>>>>>>>> self._check_query_results_editable(query, False)
>>>>>>>>> File
>>>>>>>>> "/Users/adityatoshniwal/projects/pgadmin4/web/pgadmin/feature_tests/query_tool_journey_test.py",
>>>>>>>>> line 378, in _check_query_results_editable
>>>>>>>>> is_editable = self._check_cell_editable(1)
>>>>>>>>> File
>>>>>>>>> "/Users/adityatoshniwal/projects/pgadmin4/web/pgadmin/feature_tests/query_tool_journey_test.py",
>>>>>>>>> line 395, in _check_cell_editable
>>>>>>>>> self.assertFalse('editable' in cell_classes)
>>>>>>>>> AssertionError: True is not false
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> ----------------------------------------------------------------------
>>>>>>>>> Ran 1 test in 38.118s
>>>>>>>>>
>>>>>>>>> FAILED (failures=1)
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Thanks !
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> *Yosry Muhammad Yosry*
>>>>>>>>>>
>>>>>>>>>> Computer Engineering student,
>>>>>>>>>> The Faculty of Engineering,
>>>>>>>>>> Cairo University (2021).
>>>>>>>>>> Class representative of CMP 2021.
>>>>>>>>>> https://www.linkedin.com/in/yosrym93/
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> --
>>>>>>>>> Thanks and Regards,
>>>>>>>>> Aditya Toshniwal
>>>>>>>>> Software Engineer | EnterpriseDB India | Pune
>>>>>>>>> "Don't Complain about Heat, Plant a TREE"
>>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> *Yosry Muhammad Yosry*
>>>>>>>>
>>>>>>>> Computer Engineering student,
>>>>>>>> The Faculty of Engineering,
>>>>>>>> Cairo University (2021).
>>>>>>>> Class representative of CMP 2021.
>>>>>>>> https://www.linkedin.com/in/yosrym93/
>>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> --
>>>>>>> Thanks and Regards,
>>>>>>> Aditya Toshniwal
>>>>>>> Software Engineer | EnterpriseDB India | Pune
>>>>>>> "Don't Complain about Heat, Plant a TREE"
>>>>>>>
>>>>>>
>>>>>>
>>>>>> --
>>>>>> *Yosry Muhammad Yosry*
>>>>>>
>>>>>> Computer Engineering student,
>>>>>> The Faculty of Engineering,
>>>>>> Cairo University (2021).
>>>>>> Class representative of CMP 2021.
>>>>>> https://www.linkedin.com/in/yosrym93/
>>>>>>
>>>>>
>>>>>
>>>>> --
>>>>> Thanks and Regards,
>>>>> Aditya Toshniwal
>>>>> Software Engineer | EnterpriseDB India | Pune
>>>>> "Don't Complain about Heat, Plant a TREE"
>>>>>
>>>>
>>>>
>>>> --
>>>> *Yosry Muhammad Yosry*
>>>>
>>>> Computer Engineering student,
>>>> The Faculty of Engineering,
>>>> Cairo University (2021).
>>>> Class representative of CMP 2021.
>>>> https://www.linkedin.com/in/yosrym93/
>>>>
>>>
>>>
>>> --
>>> *Yosry Muhammad Yosry*
>>>
>>> Computer Engineering student,
>>> The Faculty of Engineering,
>>> Cairo University (2021).
>>> Class representative of CMP 2021.
>>> https://www.linkedin.com/in/yosrym93/
>>>
>>
>>
>> --
>> Thanks and Regards,
>> Aditya Toshniwal
>> Software Engineer | EnterpriseDB India | Pune
>> "Don't Complain about Heat, Plant a TREE"
>>
>
>
> --
> *Yosry Muhammad Yosry*
>
> Computer Engineering student,
> The Faculty of Engineering,
> Cairo University (2021).
> Class representative of CMP 2021.
> https://www.linkedin.com/in/yosrym93/
>
--
*Thanks & Regards*
*Akshay Joshi*
*Sr. Software Architect*
*EnterpriseDB Software India Private Limited*
*Mobile: +91 976-788-8246*
