Re: [pgAdmin4][Patch]: RM #2963 - Backup database, Restore database and Maintenance Database failed for é object.

Hi Dave,

On Fri, Mar 9, 2018 at 9:09 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:

> Hi
>
> On Fri, Mar 9, 2018 at 3:32 PM, Dave Page <dpage(at)pgadmin(dot)org> wrote:
>
>> Hi
>>
>> On Fri, Mar 9, 2018 at 3:54 AM, Khushboo Vashi <
>> khushboo(dot)vashi(at)enterprisedb(dot)com> wrote:
>>
>>> Hi,
>>>
>>> Please find the attached patch to fix below issues:
>>>
>>> 1. #2963 - Backup database, Restore database and Maintenance Database
>>> failed for é object
>>> 2. #3157 - Process viewer doesn't show complete command executed.
>>>
>>> Test cases are not included for these fixes as we don't have test cases
>>> for these modules (backup, restore, maintenance).
>>> I will create one separate RM for the same which will cover this.
>>>
>>
>> Interesting that you fix these together, as together they also exhibit
>> another bug :-). Backing up the é database displays the following
>> command:
>>
>> /usr/local/pgsql/bin/pg_dump --file "/Users/dpage/foo.bak" --host
>> "localhost" --port "5432" --username "postgres" --no-password --verbose
>> --format=c --blobs "&#233;"
>>
>
> I can reproduce this issue only with notification dialogue (which I have
fixed in the attached patch) not with the details dialogue. Please refer
the screenshots for the same.

> Also, what tests can we add for backup/restore? We have nothing at all at
> the moment, and it is pretty troublesome. I'd like to ensure that we can
> backup and restore a database correctly, and ensure that the displayed
> commands are what we expect and that we get valid output from
> pg_dump/pg_restore (though, it may change from PG version to PG version, so
> maybe we should just check for something small and generic). I guess this
> might need some config parameters for the tests to specify the pg_* utility
> paths for each server.
>
> I'd suggest maybe having a feature test that opens the prefs, sets the
> appropriate path, then runs a backup, waits for it to finish, checks the
> process monitor output, then restores the same backup to a new database,
> checking the process monitor output again, and then checking that the
> restored database contains at least one object from the original database
> (we don't need to check all of pg_dump/pg_restore, just that something
> expected was restored). We should use a (partial) database name and backup
> filename from the advanced test config file, and I think both should
> default to some interesting non-ASCII strings to ensure quoting works.
>
I was thinking of writing the unit test cases for the processes.py file as
all the major functionalities for backup/restore/maintenance jobs done by
this file, but by this we can not achieve the front-end string validation
esp for non-ASCII strings.
So, I am thinking of writing feature tests (as you have suggested) first
and after that if needed I will write unit test cases.

>
> --
> Dave Page
> Blog: http://pgsnake.blogspot.com
> Twitter: @pgsnake
>
> EnterpriseDB UK: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>