Great! thanks everyone.
On Wed, Aug 14, 2019, 10:08 AM Akshay Joshi <akshay.jo...@enterprisedb.com>
wrote:
> 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*
>
