Hello Akshay, Thanks for taking a look. We did the following changes over your patch:
- Changed the XPATH to CSS_SELECTOR, please look in pgadmin/feature_tests/query_tool_tests.py:660 and 667 - Changed the other places in _query_tool_notify_statements to do not use xpath. - Moved the skip that previously was around the entire function to surround only the pg_notify call Thanks Victoria && Joao On Wed, May 30, 2018 at 1:52 AM Akshay Joshi <akshay.jo...@enterprisedb.com> wrote: > Hi Joao > > On Tue, May 29, 2018 at 9:10 PM, Joao De Almeida Pereira < > jdealmeidapere...@pivotal.io> wrote: > >> Hello Akshay, >> >> The code looks good, we do have some minor notes for next time: >> >> . To make the code easier to understand we should decide on 1 term and >> not have notifies and notifications to reference the same thing. >> > Fixed and used 'notifies' where it reference the same, but still at > some places 'notifications' is used because the new tab represents the > 'Notifications'. > > >> . It would be nice if we tried not to use XPATH to get the HTML >> components that we are testing on. Maybe try to use CSS classes. Selenium >> also has a driver.select_by_class_name which would work well in this >> case. >> > Tried to use both CSS_SELECTOR and CLASS_NAME, but it doesn't > support string comparison(contains method). I have googled for this and > most of the sites suggested to use XPATH where we can use contains() > method. > > >> The error that we are seeing in CI is related to pg_notify. This function >> was introduced in 9.0 and Greenplum is still not there yet. In order to >> solve this need to skip that part of the tests. There is an example in that >> same file using the function _test_explain_plan_feature to skip a tests >> depending on the Version. >> > OK. I have skip that part of the tests as per your suggestion. I have > rename the method "_test_explain_plan_feature" to > "_supported_server_version" which is more meaningful then the previous one. > > Attached is the modified patch. Please review it. > > >> Thanks >> Anthony && Joao >> >> >> On Mon, May 28, 2018 at 1:34 AM Akshay Joshi < >> akshay.jo...@enterprisedb.com> wrote: >> >>> Hi Hakers, >>> >>> On my last patch feature tests were failed for GreenPlum5 database only >>> not for PostgreSQL. I have verified that on >>> https://gpdb-dev.bosh.pivotalci.info/teams/pgadmin/pipelines/pgadmin-patch/jobs/run-tests/builds/93 >>> . >>> >>> Can someone from Pivotal team help me, as I don't have GreenPlum >>> database and don't know why it is failing. >>> >>> On Sat, May 26, 2018 at 5:47 PM, Akshay Joshi < >>> akshay.jo...@enterprisedb.com> wrote: >>> >>>> Hi Hackers, >>>> >>>> As per suggestion by Dave and discussion with in the team, I have >>>> modified the logic again. Following are the modifications: >>>> >>>> - Instead of waiting for another query to execute on the session >>>> where 'LISTEN' command has been executed, we fetched the notify >>>> messages in >>>> the connection status polling logic. Doing this user will get the notify >>>> messages asap. >>>> - Instead of showing all the notifications in single alertify dialog, >>>> we call *alertify.info <http://alertify.info>('<msg>') *for >>>> individual notifications. >>>> - Created new tab 'Notifications' in Query Tool where all the >>>> notify messages will be recorded with the timestamp and payload. >>>> >>>> Please review the latest patch. >>>> >>>> On Tue, May 22, 2018 at 2:43 PM, Dave Page <dp...@pgadmin.org> wrote: >>>> >>>>> >>>>> >>>>> On Tue, May 22, 2018 at 10:01 AM, Akshay Joshi < >>>>> akshay.jo...@enterprisedb.com> wrote: >>>>> >>>>>> Hi Dave >>>>>> >>>>>> On Tue, May 22, 2018 at 2:02 PM, Dave Page <dp...@pgadmin.org> wrote: >>>>>> >>>>>>> >>>>>>> >>>>>>> On Tue, May 22, 2018 at 9:13 AM, Dave Page <dp...@pgadmin.org> >>>>>>> wrote: >>>>>>> >>>>>>>> Hi >>>>>>>> >>>>>>>> On Tue, May 22, 2018 at 7:07 AM, Akshay Joshi < >>>>>>>> akshay.jo...@enterprisedb.com> wrote: >>>>>>>> >>>>>>>>> Hi Hackers, >>>>>>>>> >>>>>>>>> As per suggestion by Dave, I have modified the logic and now >>>>>>>>> notifications are popped up in alertify dialog(refer >>>>>>>>> Notify_Messages.png) as and when received on that session where >>>>>>>>> "LISTEN" is executed. Attached is the modified patch, please review >>>>>>>>> it. >>>>>>>>> >>>>>>>>> To test this feature following steps need to perform: >>>>>>>>> >>>>>>>>> - Apply the patch. >>>>>>>>> - Run pgAdmin4 >>>>>>>>> - Connect to any database server and open query tool. >>>>>>>>> - Execute 'LISTEN foo;' command. >>>>>>>>> - Open another query tool window and execute 'NOTIFY foo'. >>>>>>>>> (This is without payload). >>>>>>>>> - Execute 'select pg_notify('foo', 'Hello')' query (with >>>>>>>>> payload). >>>>>>>>> - Go to the query tool window from where 'LISTEN' was executed >>>>>>>>> and run any other query. >>>>>>>>> >>>>>>>>> I think there was a small misunderstanding here - I was suggesting >>>>>>>> that each notification be displayed in an Alertify notification, e.g. >>>>>>>> using >>>>>>>> alertify.message('A notification of FOO was received with payload >>>>>>>> '1234'...') >>>>>>>> >>>>>>> >>>>>> If there are too many notifications then it's annoying for >>>>>> user to popped up N number of alertify dialogs. Notification is only >>>>>> receives when any other query execute on the session where "LISTEN" >>>>>> command >>>>>> executes. So for example I have NOTIFY 10 times from different sessions >>>>>> and >>>>>> execute any other query on the session("LISTEN" one), 10 alertify >>>>>> dialog will be popped up. >>>>>> >>>>> >>>>> Sure, but then a) it's the users choice to listen for something very >>>>> noisey, and b) if there are many notifications then the message box >>>>> dialogue will become huge. >>>>> >>>>> The other nice thing about using notifications is that they don't >>>>> require any acknowledgement; they show you the event happened, and then >>>>> get >>>>> out of the way, allowing you to jump to the messages tab if needed. >>>>> >>>>> >>>>>> >>>>>> >>>>>>> >>>>>>> And it failed tests: >>>>>>> https://gpdb-dev.bosh.pivotalci.info/teams/pgadmin/pipelines/pgadmin-patch/jobs/run-tests/builds/85 >>>>>>> :-( >>>>>>> >>>>>>> Again it's timeout issue, not able to reproduce on my machine >>>>>> will look into it. Maybe will have to add webDriverWait. >>>>>> >>>>> >>>>> OK. >>>>> >>>>> >>>>>> >>>>>>>> >>>>>>>> >>>>>>>>> >>>>>>>>> - >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> On Mon, May 21, 2018 at 1:36 PM, Akshay Joshi < >>>>>>>>> akshay.jo...@enterprisedb.com> wrote: >>>>>>>>> >>>>>>>>>> Hi Dave >>>>>>>>>> >>>>>>>>>> On Fri, May 18, 2018 at 4:56 PM, Dave Page <dp...@pgadmin.org> >>>>>>>>>> wrote: >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> On Fri, May 18, 2018 at 12:11 PM, Akshay Joshi < >>>>>>>>>>> akshay.jo...@enterprisedb.com> wrote: >>>>>>>>>>> >>>>>>>>>>>> Hi Dave >>>>>>>>>>>> >>>>>>>>>>>> On Fri, May 18, 2018 at 3:58 PM, Dave Page <dp...@pgadmin.org> >>>>>>>>>>>> wrote: >>>>>>>>>>>> >>>>>>>>>>>>> Hi >>>>>>>>>>>>> >>>>>>>>>>>>> On Wed, May 16, 2018 at 2:51 PM, Anthony Emengo < >>>>>>>>>>>>> aeme...@pivotal.io> wrote: >>>>>>>>>>>>> >>>>>>>>>>>>>> Hey, >>>>>>>>>>>>>> >>>>>>>>>>>>>> The code looks great! The tests all passed as well. >>>>>>>>>>>>>> >>>>>>>>>>>>> >>>>>>>>>>>>> Agreed - however, unless you check the Messages panel, you're >>>>>>>>>>>>> not likely to see that a message was received. >>>>>>>>>>>>> >>>>>>>>>>>>> Can we also show each message in an alertify panel? >>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> We need to change the design I guess, as we are currently >>>>>>>>>>>> send this as part of Messages. We will have to send this >>>>>>>>>>>> separately and >>>>>>>>>>>> show it in the alertify panel. >>>>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> Yeah. Unfortunately I think notifications need to be more >>>>>>>>>>> "active" than the messages. >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> I am working on the above. Can we add one preferences setting >>>>>>>>>> to "ON/OFF" this alertify panel ? >>>>>>>>>> >>>>>>>>>>> >>>>>>>>>>> -- >>>>>>>>>>> Dave Page >>>>>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>>>>> Twitter: @pgsnake >>>>>>>>>>> >>>>>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>>>>> The Enterprise PostgreSQL Company >>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> *Akshay Joshi* >>>>>>>>>> >>>>>>>>>> *Sr. Software Architect * >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> *Akshay Joshi* >>>>>>>>> >>>>>>>>> *Sr. Software Architect * >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> Dave Page >>>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>>> Twitter: @pgsnake >>>>>>>> >>>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>>> The Enterprise PostgreSQL Company >>>>>>>> >>>>>>> >>>>>>> >>>>>>> >>>>>>> -- >>>>>>> Dave Page >>>>>>> Blog: http://pgsnake.blogspot.com >>>>>>> Twitter: @pgsnake >>>>>>> >>>>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>>>> The Enterprise PostgreSQL Company >>>>>>> >>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> *Akshay Joshi* >>>>>> >>>>>> *Sr. Software Architect * >>>>>> >>>>>> >>>>>> >>>>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >>>>>> >>>>> >>>>> >>>>> >>>>> -- >>>>> Dave Page >>>>> Blog: http://pgsnake.blogspot.com >>>>> Twitter: @pgsnake >>>>> >>>>> EnterpriseDB UK: http://www.enterprisedb.com >>>>> The Enterprise PostgreSQL Company >>>>> >>>> >>>> >>>> >>>> -- >>>> *Akshay Joshi* >>>> >>>> *Sr. Software Architect * >>>> >>>> >>>> >>>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >>>> >>> >>> >>> >>> -- >>> *Akshay Joshi* >>> >>> *Sr. Software Architect * >>> >>> >>> >>> *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >>> >> > > > -- > *Akshay Joshi* > > *Sr. Software Architect * > > > > *Phone: +91 20-3058-9517Mobile: +91 976-788-8246* >
RM_3204_v7.patch
Description: Binary data
