Thanks patch applied. On Mon, May 15, 2017 at 6:19 AM, Matthew Kleiman <mklei...@pivotal.io> wrote:
> Hi Akshay > > I really appreciate you taking a look at this on a Sunday! > I am not able to get access to my work computer to run your changes > directly. From what you are saying, though, it sounds like a reasonable > change. If this change is passing in your environment, I say go ahead and > make the change to the patch and commit. > > Thanks again for helping us get this into the release. > > Best, > Matt > > On Sun, May 14, 2017 at 4:52 AM, Akshay Joshi < > akshay.jo...@enterprisedb.com> wrote: > >> Hi Joao >> >> On Fri, May 12, 2017 at 11:23 PM, Joao Pedro De Almeida Pereira < >> jdealmeidapere...@pivotal.io> wrote: >> >>> Hello Akshay, >>> >>> You can find attached a new version of the patch that changes the test >>> in question. >>> This new version instead of requiring a creation of a new Role now does >>> the following: >>> 1 - Creates a new Role >>> 2 - Creates a table with that Role >>> 3 - Checks for dependencies >>> >> >> Test case still failing on my machine because when we create new Role >> it is without password and when we try to create table with that role first >> we will try to connect the database using that new role with >> servers['db_password'] which is wrong. I have modified setup function of " >> *test_role_dependencies_sql.py*" and it works, we can create new role >> using database server's password >> >> Old setup function: >> *def *setUp(self): >> >> create_role(self.server, *"testpgadmin"*) >> self.server_with_modified_user = self.server.copy() >> self.server_with_modified_user[*'username'*] = *"testpgadmin"* >> >> >> Modified setup function: >> >> >> def setUp(self): >> >> with test_utils.Database(self.server) as (connection, database_name): >> cursor = connection.cursor() >> try: >> cursor.execute( >> "CREATE ROLE testpgadmin LOGIN PASSWORD '%s'" >> % self.server['db_password']) >> except Exception as exception: >> print(exception) >> connection.commit() >> >> self.server_with_modified_user = self.server.copy() >> self.server_with_modified_user['username'] = "testpgadmin" >> >> >> If the above logic looks good to you then I'll commit the code. >> >>> >>> Thanks >>> Joao & George >>> >>> On Thu, May 11, 2017 at 7:33 AM, Joao Pedro De Almeida Pereira < >>> jdealmeidapere...@pivotal.io> wrote: >>> >>>> Hello all, >>>> This test is trying to check if the sql to retrieve the role >>>> dependencies of a object in the database is correct. Using a different user >>>> to connect to the database and create tables there is no need for extra >>>> setup because the role depedencies table entry is added automatically. Also >>>> we talked with some internal users and they told us they always used a >>>> different user to connect to the database with pgadmin. >>>> >>>> After your comments we realize that the test need to have a different >>>> setup in order to become more readable and resilient. What do we need to do >>>> in the test setup to ensure an entry is added to the role dependency table? >>>> >>>> Also the test need to be a little bit more resilient to work >>>> independently of the user that is used on the test. >>>> >>>> Thanks >>>> Joao >>>> >>>> On Thu, May 11, 2017, 3:20 AM Khushboo Vashi < >>>> khushboo.va...@enterprisedb.com> wrote: >>>> >>>>> On Thu, May 11, 2017 at 12:03 PM, Dave Page <dp...@pgadmin.org> wrote: >>>>> >>>>>> >>>>>> >>>>>> On 11 May 2017, at 07:11, Akshay Joshi <akshay.jo...@enterprisedb.com> >>>>>> wrote: >>>>>> >>>>>> Hi Sarah >>>>>> >>>>>> On Thu, May 11, 2017 at 2:30 AM, Sarah McAlear <smcal...@pivotal.io> >>>>>> wrote: >>>>>> >>>>>>> Hi Ashkay! >>>>>>> >>>>>>> TestTablesNodeSql hasn't failed but TestRoleDependenciesSql failed >>>>>>>> with below error: >>>>>>>> .....\test_role_dependencies_sql.py\", line 41, in assertions\n >>>>>>>> self.assertEqual(1, len(fetch_result))\nAssertionError: 1 != 0 >>>>>>> >>>>>>> >>>>>>> We experienced a similar problem when we started developing this >>>>>>> patch. We noticed that we were connecting to the database with the super >>>>>>> user (the db owner user), which did not create the role we are expecting >>>>>>> when a table is created. To ensure that this role is created, we had to >>>>>>> create a new user and use it to execute all the tests. After we made >>>>>>> this >>>>>>> change, the tests passed. >>>>>>> >>>>>>> Can you try to create a new user and use it to execute your tests? >>>>>>> >>>>>> >>>>>> I did that and it works. I have created new user 'test' with >>>>>> superuser privileges and update parameter db_username="test" in >>>>>> test_config.json file. But still I am unable to understand why it doesn't >>>>>> work with 'postgres' (default) user? >>>>>> >>>>>> >>>>>> Agreed. That suggests something fishy is going on that should be >>>>>> understood. >>>>>> >>>>>> >>>>> We don't check role dependencies for every object. As per the code, >>>>> the role dependencies are executed for columns not for the table itself. >>>>> And in the test cases, the table object is trying to execute the role >>>>> dependencies, so I think this test case is not correct. >>>>> >>>>>> >>>>>>> Thanks, >>>>>>> João & Sarah >>>>>>> >>>>>>> On Wed, May 10, 2017 at 1:56 PM, Akshay Joshi < >>>>>>> akshay.jo...@enterprisedb.com> wrote: >>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On May 10, 2017 19:07, "Sarah McAlear" <smcal...@pivotal.io> wrote: >>>>>>>> >>>>>>>> Hi Akshay! >>>>>>>> >>>>>>>> Does this error occur on 9.6 or 10.0? We tested it in 9.6 and all >>>>>>>> our tests pass. >>>>>>>> >>>>>>>> >>>>>>>> On both 9.6 and 10.0. Query returned 0 rows which is compared >>>>>>>> against 1 in assert statement. >>>>>>>> >>>>>>>> >>>>>>>> Thanks! >>>>>>>> João & Sarah >>>>>>>> >>>>>>>> On Wed, May 10, 2017 at 2:29 AM, Akshay Joshi < >>>>>>>> akshay.jo...@enterprisedb.com> wrote: >>>>>>>> >>>>>>>>> Hi Sarah >>>>>>>>> >>>>>>>>> On Tue, May 9, 2017 at 9:03 PM, Sarah McAlear <smcal...@pivotal.io >>>>>>>>> > wrote: >>>>>>>>> >>>>>>>>>> Hi Akshay! >>>>>>>>>> >>>>>>>>>> >>>>>>>>>>> Some test file names ended with "*_sql_template.py*" do we need >>>>>>>>>>> to add that string ? >>>>>>>>>> >>>>>>>>>> we added this suffix after moving the tests up a level to >>>>>>>>>> tables/tests to clarify what subject they applied to. we changed the >>>>>>>>>> suffix >>>>>>>>>> to "_sql.py" >>>>>>>>>> >>>>>>>>>> - Files "test_column_acl_sql_template.py" and " >>>>>>>>>>> test_column_properties_sql_template.py" should be moved from >>>>>>>>>>> tables->tests to tables->column->tests. As it's related to column. >>>>>>>>>>> - Files "test_trigger_get_oid_sql_template.py" and " >>>>>>>>>>> test_trigger_nodes_sql_template.py" should be moved from >>>>>>>>>>> tables->tests to tables->triggers->tests. As its related to >>>>>>>>>>> triggers. >>>>>>>>>> >>>>>>>>>> these tests are related to the sql files in >>>>>>>>>> tables/templates/column, not tables/column, so moving them into >>>>>>>>>> tables/column would be more confusing. >>>>>>>>>> >>>>>>>>>> Following test cases are failing >>>>>>>>>> >>>>>>>>>> Thank you, fixed, see new patch. Can you confirm that >>>>>>>>>> TestTablesNodeSql doesn't fail in your environment? >>>>>>>>>> >>>>>>>>> >>>>>>>>> TestTablesNodeSql hasn't failed but TestRoleDependenciesSql >>>>>>>>> failed with below error: >>>>>>>>> .....\test_role_dependencies_sql.py\", line 41, in assertions >>>>>>>>> \n self.assertEqual(1, len(fetch_result))\nAssertionError: 1 != 0 >>>>>>>>> >>>>>>>>> >>>>>>>>>> Thanks, >>>>>>>>>> George & Sarah >>>>>>>>>> >>>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> -- >>>>>>>>> *Akshay Joshi* >>>>>>>>> *Principal Software Engineer * >>>>>>>>> >>>>>>>>> >>>>>>>>> >>>>>>>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91 >>>>>>>>> 976-788-8246 <+91%2097678%2088246>* >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> *Akshay Joshi* >>>>>> *Principal Software Engineer * >>>>>> >>>>>> >>>>>> >>>>>> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91 >>>>>> 976-788-8246 <+91%2097678%2088246>* >>>>>> >>>>>> >>> >> >> >> -- >> *Akshay Joshi* >> *Principal Software Engineer * >> >> >> >> *Phone: +91 20-3058-9517 <+91%2020%203058%209517>Mobile: +91 976-788-8246 >> <+91%2097678%2088246>* >> > > -- *Akshay Joshi* *Principal Software Engineer * *Phone: +91 20-3058-9517Mobile: +91 976-788-8246*
