Re: [pgAdmin4][RM#3271] Update to the latest 3.x version of jQuery

From: Joao De Almeida Pereira <jdealmeidapereira(at)pivotal(dot)io>
To: Aditya Toshniwal <aditya(dot)toshniwal(at)enterprisedb(dot)com>
Cc: pgadmin-hackers <pgadmin-hackers(at)postgresql(dot)org>
Subject: Re: [pgAdmin4][RM#3271] Update to the latest 3.x version of jQuery
Date: 2018-05-24 19:00:14
Message-ID: CAE+jja=L6Cvs35Lx-+a6yJ1xRH=mjYebvQGdWxr_-E0PCf6TDQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgadmin-hackers

Hello Aditya,

We changed the way the tests were running and we could remove the function.
Instead of piggybackiung on a deprecated field we created a html fixture

$('body').append(
'<div id="test-id">' +
'<button id="btn-flash" disabled></button>' +
'<button id="btn-cancel-query"></button>' +
'</div>'
);

With it we could use the real JQuery instead of mocking it.
Now the tests look like this

it('disables the run query button', () => {
let buttonFlash = $('#btn-flash');

expect(buttonFlash.prop('disabled')).toEqual(true);
});

Thanks
Anthony & Joao

On Thu, May 24, 2018 at 12:26 AM Aditya Toshniwal <
aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:

> Hi,
>
> No luck with data-test-selector. :(
> I added data-test-selector to html tags. I get undefined for
> .data('test-selector') as well as for .attr('data-test-selector').
>
> Thanks and Regards,
> Aditya Toshniwal
> Software Engineer | EnterpriseDB Software Solutions | Pune
> "Don't Complain about Heat, Plant a tree"
>
> On Thu, May 24, 2018 at 9:17 AM, Aditya Toshniwal <
> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>
>> Hi Victoria/Joao,
>>
>> Thank you for reviewing. The above function is probably the last solution
>> which I added.
>> The test cases
>> in web/regression/javascript/sqleditor/execute_query_spec.js calls the
>> execute query function of pgAdmin4 sqleditor. The execution
>> disables/enables the buttons internally and is not done by the test cases.
>> So there is spy on prop function (jqueryPropSpy = spyOn($.fn, 'prop')) of
>> jQuery. The problem is, the spy jQuery object (in function
>> findJQueryCallWithSelector) returned seems to have very less information,
>> not sure why. I had tried using css selector for id, tried extracting id
>> directly, class, but none of them were present in the object.
>>
>> I can try adding data-test-selector to the HTML5 tag, if it works then
>> will send the updated patch.
>>
>>
>> Thanks and Regards,
>> Aditya Toshniwal
>> Software Engineer | EnterpriseDB Software Solutions | Pune
>> "Don't Complain about Heat, Plant a tree"
>>
>> On Wed, May 23, 2018 at 7:41 PM, Joao De Almeida Pereira <
>> jdealmeidapereira(at)pivotal(dot)io> wrote:
>>
>>> HI Aditya,
>>>
>>> Good job porting the app from the ancient version of JQuery to the
>>> latest build :D
>>>
>>> The patch in general looks good, and the patches-bot says all tests
>>> pass.
>>>
>>> The only thing that we would like to point out is this piece of code:
>>>
>>> /* jQuery has removed selector property in version 3.x
>>> * To allow JS test cases to run, the below function is
>>> * used to set the selector property
>>> */
>>> function setPropAndSelector(selector, propName, propVal) {
>>> let tmpObj = $(selector);
>>> tmpObj['selector'] = selector;
>>> tmpObj.prop(propName,propVal);
>>> return tmpObj;
>>> }
>>>
>>> As tests are also part of the code we believe that when there is a
>>> change that affect testing we should change the tests to work accordingly.
>>> So our suggestion is, instead of creating this function, why don’t we use a
>>> CSS selector in the tests to do the clicking or even use the HTML5 tag
>>> data-test-selector?
>>> This way the tests will also be coherent with the upgrade from JQuery 1
>>> to 3
>>> ​
>>>
>>> Thanks
>>> Victoria & Joao
>>>
>>> On Wed, May 23, 2018 at 6:44 AM Aditya Toshniwal <
>>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>>
>>>> Hi Hackers,
>>>>
>>>> Please find the updated patch for upgrading the jQuery version to 3.x.
>>>> The patch is only for pgAdmin4 code changes replacing old deprecated
>>>> functions with new replacement.
>>>> We need to look into external libraries used for any vulnerabilities.
>>>> Request you to kindly review.
>>>>
>>>> Thanks and Regards,
>>>> Aditya Toshniwal
>>>> Software Engineer | EnterpriseDB Software Solutions | Pune
>>>> "Don't Complain about Heat, Plant a tree"
>>>>
>>>> On Mon, May 14, 2018 at 1:23 PM, Aditya Toshniwal <
>>>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>>>
>>>>> Hi Hackers,
>>>>>
>>>>> Please hold on with the patch. Will send the updated patch soon.
>>>>> Apologies.
>>>>>
>>>>>
>>>>>
>>>>> Thanks and Regards,
>>>>> Aditya Toshniwal
>>>>> Software Engineer | EnterpriseDB Software Solutions | Pune
>>>>> "Don't Complain about Heat, Plant a tree"
>>>>>
>>>>> On Mon, May 14, 2018 at 12:52 PM, Aditya Toshniwal <
>>>>> aditya(dot)toshniwal(at)enterprisedb(dot)com> wrote:
>>>>>
>>>>>> Hi Hackers,
>>>>>>
>>>>>> PFA patch for upgrading jQuery version to 3.3.1 from current version
>>>>>> 1.12.4. Patch includes replacing deprecated functions of jquery to latest
>>>>>> one.
>>>>>> Kindly review.
>>>>>>
>>>>>> Thanks and Regards,
>>>>>> Aditya Toshniwal
>>>>>> Software Engineer | EnterpriseDB Software Solutions | Pune
>>>>>> "Don't Complain about Heat, Plant a tree"
>>>>>>
>>>>>
>>>>>
>>>>
>>
>

Attachment Content-Type Size
0001-rm-3271-v3.patch text/x-patch 45.2 KB

In response to

Responses

Browse pgadmin-hackers by date

  From Date Subject
Next Message Joao De Almeida Pereira 2018-05-24 19:36:37 Re: [pgadmin4][patch] Use pytest test runner for unit tests
Previous Message Dave Page 2018-05-24 16:24:17 Re: [pgadmin4][patch] Use pytest test runner for unit tests