Re: [GSoC] Query History Upgrade

Any more suggestions or comments on the patch?

On Tue, Aug 13, 2019 at 2:55 PM Aditya Toshniwal <
aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:

>
>
> On Tue, Aug 13, 2019 at 5:35 PM Yosry Muhammad <yosrym93(at)gmail(dot)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 <yosrym93(at)gmail(dot)com>
>> wrote:
>>
>>> Is it always the same error when it fails?
>>>
>>> On Tue, Aug 13, 2019 at 1:50 PM Aditya Toshniwal <
>>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>>
>>>> The test cases fails intermittently. It passes sometimes.
>>>>
>>>> On Tue, Aug 13, 2019 at 5:15 PM Yosry Muhammad <yosrym93(at)gmail(dot)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(dot)toshniwal(at)enterprisedb(dot)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 <yosrym93(at)gmail(dot)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(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>>>>>>
>>>>>>>> Hi Yosry,
>>>>>>>>
>>>>>>>>
>>>>>>>> On Mon, Aug 12, 2019 at 2:00 PM Yosry Muhammad <yosrym93(at)gmail(dot)com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>> Hi Aditya,
>>>>>>>>>
>>>>>>>>> On Mon, Aug 12, 2019 at 9:04 AM Aditya Toshniwal <
>>>>>>>>> aditya(dot)toshniwal(at)enterprisedb(dot)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/