Received: from malur.postgresql.org ([2a02:16a8:dc51::56]) by arkaria.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fMB18-0005kp-TH for pgadmin-hackers@arkaria.postgresql.org; Fri, 25 May 2018 11:36:19 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.89) (envelope-from ) id 1fMB16-0001ga-Qb for pgadmin-hackers@arkaria.postgresql.org; Fri, 25 May 2018 11:36:16 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fMB16-0001gQ-LT for pgadmin-hackers@lists.postgresql.org; Fri, 25 May 2018 11:36:16 +0000 Received: from mx0a-00296801.pphosted.com ([148.163.150.38]) by magus.postgresql.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_CBC_SHA384:256) (Exim 4.89) (envelope-from ) id 1fMB11-0003AR-U3 for pgadmin-hackers@postgresql.org; Fri, 25 May 2018 11:36:15 +0000 Received: from pps.filterd (m0114583.ppops.net [127.0.0.1]) by mx0a-00296801.pphosted.com (8.16.0.22/8.16.0.22) with SMTP id w4PBa84X003743 for ; Fri, 25 May 2018 11:36:08 GMT Received: from mail-io0-f197.google.com (mail-io0-f197.google.com [209.85.223.197]) by mx0a-00296801.pphosted.com with ESMTP id 2j2c8wq8xk-1 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128 verify=NOT) for ; Fri, 25 May 2018 11:36:08 +0000 Received: by mail-io0-f197.google.com with SMTP id s2-v6so4007038ioa.22 for ; Fri, 25 May 2018 04:36:08 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=3mjmzcVh4e+8WN7hlcWUN9PQ9xrxXq0fMDUOIVVTW9w=; b=BI98JQjzS5B2/n78IJrHpoGi0EikS2ADrfqLi2KkvOs87gFpQpSMe/95wa1+/y9Umn iOOAoul22wWJw8nd07Oy2dGnQjPOmyLpprdfVw6s4JQ4l3ijNF/lj4i6Xnlah2fpKEix UEOby+Oy1B5MgivZBRQABVkotRQkCRWSecLTMl9rjGF/ffIVppqAZbDudF4aM5LMP6Qd nw2SwkWU/u1LTks38+/JaiHK1gTEzxsJTiwD+TZ9CDgic3SUevTc5KMOCWOWQmoN2+I/ +11fR04cuKM5hsXKoJNOvOYhruZT77mcPWZD64Nq57hyuqofW3qwvxHtvLgtU3lipyhv 4qFg== X-Gm-Message-State: ALKqPwfRA6VdbRiDvNtiJvoB06lcvjOkZsjl89wM1ifzX5nQQYAHcVY/ YATKGypVMWfm+qkyzMj7S29f48FTfXBxdfHqEwaahDTMnegnvmftMeFIPsDubBErayaeNlMQxTG 75LG5NQFpXUY/a3dnGbLB4cAqmvgDeTTx+MePljXDJ/B+8GZ+A6D3pUXfQjFNWTX847hH X-Received: by 2002:a24:10c5:: with SMTP id 188-v6mr1708615ity.69.1527248167362; Fri, 25 May 2018 04:36:07 -0700 (PDT) X-Google-Smtp-Source: ADUXVKIoUuG+UZ7ahOTwN3/zxslkFd+lxgNOtL2m43PZaobBvP/8w/j7LG9a4PCslxhGXiz349rKhH64JcnxnrtdKEM= X-Received: by 2002:a24:10c5:: with SMTP id 188-v6mr1708598ity.69.1527248167166; Fri, 25 May 2018 04:36:07 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: Joao De Almeida Pereira Date: Fri, 25 May 2018 07:35:53 -0400 Message-ID: Subject: Re: [pgadmin4][patch] Use pytest test runner for unit tests To: Dave Page Cc: Anthony Emengo , pgadmin-hackers Content-Type: multipart/alternative; boundary="0000000000007d0196056d062cc9" X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-05-25_04:,, signatures=0 X-Proofpoint-Spam-Details: rule=notspam policy=default score=0 priorityscore=1501 malwarescore=0 suspectscore=1 phishscore=0 bulkscore=0 spamscore=0 clxscore=1015 lowpriorityscore=0 mlxscore=0 impostorscore=0 mlxlogscore=999 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1711220000 definitions=main-1805250127 List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Precedence: bulk --0000000000007d0196056d062cc9 Content-Type: text/plain; charset="UTF-8" Hello Dave > As part of the development environment we do not see the reasoning behind >> not add PYTHONPATH to the environment variables, specially because this >> looks like the way pytest was invisoned. >> > > Really? It's one more step that wasn't previously required, and for which > there is no good reason when running in a properly configured virtual > environment. Not only that, but PYTHONPATH is typically used as a search > path to find modules on which the application/script is dependent - not to > find the application/script itself. Unconditionally overriding it is likely > to lead to problems in some possible configurations (at the very least I > would expect to see the Makefile do PYTHONPATH=$PYTHONPATH:$(PWD)/web). > Good point we didn't consider the possibility of someone developing multiple python apps in the same machine. We will revisit this and start using python -m pytest as that should solve the problem > > >> >> However please try the following patch instead. We've changed the pytest >> invocation to assume the relevant dir as part of the directories to load, >> as well as the docs and Makefile >> > > Some initial feedback: > > - The JSON results are no longer being generated. > That is a flag that we didn't add to the script bug we will review the patch with that > - The output is *extremely* verbose, showing a lot of seemingly > unnecessary info (do we really need to output the source code of failing > test cases?). I would suggest that the verbose output be directed to the > log, and the visible output should be much more terse along the lines of > what it is now. > We can see what we can do about this point > - There is no summary at the end showing what passed, failed or was > skipped. To figure out what to look at, I have to trawl through over 13K > lines of output (642KB). > If we use the flag -q the output is smaller. We can add that too > - 69 tests failed for me running test:unit. They were previously all > passing. > Can you provide some log of the failing tests? > - It is a *lot* faster - not sure if that's a result of tests failing, but > I expect not entirely. > > - /README was updated, but not /web/regression/README > Out lapse, will do that Joao --0000000000007d0196056d062cc9 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello Dave
As part of= the development environment we do not see the reasoning behind not add PYT= HONPATH to the environment variables, specially because this looks like the= way pytest was invisoned.

Really? It's one more step that wasn't previously require= d, and for which there is no good reason when running in a properly configu= red virtual environment. Not only that, but PYTHONPATH is typically used as= a search path to find modules on which the application/script is dependent= - not to find the application/script itself. Unconditionally overriding it= is likely to lead to problems in some possible configurations (at the very= least I would expect to see the Makefile do PYTHONPATH=3D$PYTHONPATH:$(PWD= )/web).
Good point we didn&#= 39;t consider the possibility of someone developing multiple python apps in= the same machine.
We will revisit this and start using=C2=A0
python -m pytest=C2=A0
as that should solve the problem
=C2=A0
<= blockquote class=3D"gmail_quote" style=3D"margin:0px 0px 0px 0.8ex;border-l= eft:1px solid rgb(204,204,204);padding-left:1ex">

=
However please try the following patch instead. We've changed the= pytest invocation to assume the relevant dir as part of the directories to= load, as well as the docs and Makefile

Some initial feedback:

- The JSON= results are no longer being generated.

That is a flag = that we didn't add to the script bug we will review the patch with that=

=

- The output is *extremely* verbose, showing a lot of s= eemingly unnecessary info (do we really need to output the source code of f= ailing test cases?).=C2=A0I would suggest that th= e verbose output be directed to the log, and the visible output should be m= uch more terse along the lines of what it is now.
<= /div>

We can see what we can do about= this point


- There is no summary at the end showing wha= t passed, failed or was skipped. To figure out what to look at, I have to t= rawl through over 13K lines of output (642KB).

If we use the flag -q the output is small= er. We can add that too


- 69 tests failed for me running= test:unit. They were previously all passing.

Can you provide some log of the failing te= sts?


- It is a *lot* faster - not sure if that's a r= esult of tests failing, but I expect not entirely.

- /README was updated, but not /web/regression/README

Out lapse, will do that

Joao
--0000000000007d0196056d062cc9--