Re: [HACKERS] Automatic testing of patches in commit fest

[Aleksander Alekseev]

Hi Thomas,

> 1.  I didn't have --enable-cassert enabled before.  Oops.
> 2.  It'll now dump a gdb backtrace for any core files found after a
> check-world failure (if you can find your way to the build log...).
> Thanks to Andres for the GDB scripting for this!
> 3.  It'll now push gcov results to codecov.io -- see link at top of
> page.  Thanks again to Andres for this idea.
> 4.  It now builds a little bit faster due to -j4 (Travis CI VMs have 2
> virtual cores) and .proverc -j3.  (So far one entry now fails in TAP
> tests with that setting, will wait longer before drawing any
> conclusions about that.)

Wow. Well done!

> LLVM sanitizer stuff

In my experience it doesn't work well with PostgreSQL code, way to many
false positives. For more details see this discussion [1].

> Valgrind

That would be great. Please note that Valgrind generates false reports
regarding memory leaks in PostgreSQL so you should use --leak-check=no.
Also USE_VALGRIND should be defined in src/include/pg_config_manual.h
before building PostgreSQL. Here [2] is a script I'm using.

> Coverity

I believe it would be not easy to integrate with the web-version of
Coverity. On the other hand Clang Static Analyzer often finds real
defects and it's very easy to integrate with it. Here is my script [3].

> more compilers, 

In my experience trying to compile a patch on FreeBSD with Clang often
helps to find some sort of defect. Same regarding trying different
architectures, e.g. x64, x86, arm.

[1]: https://www.postgresql.org/message-id/20160321130850.6ed6f598%40fujitsu
[2]: https://github.com/afiskon/pgscripts/blob/master/valgrind.sh
[3]: https://github.com/afiskon/pgscripts/blob/master/static-analysis.sh

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Automatic testing of patches in commit fest

[Thomas Munro]

On Mon, Sep 18, 2017 at 2:39 PM, Andres Freund  wrote:
> E.g. very little of the new stuff in 
> https://codecov.io/gh/postgresql-cfbot/postgresql/commit/ceaa3dbece3c9b98abcaa28009320fde45a83f88
> is exercised.

Hoist by my own petard.

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Andres Freund]

Hi,

On 2017-09-18 14:26:53 +1200, Thomas Munro wrote:
>  A couple of new experimental features on commitfest.cputube.org:

Yay.


> 2.  It'll now dump a gdb backtrace for any core files found after a
> check-world failure (if you can find your way to the build log...).
> Thanks to Andres for the GDB scripting for this!

Scripting that should not be needed, except that tools are generally
crappy :(


> The code coverage reports at codecov.io are supposed to allow
> comparison between a branch and master so you can see exactly what
> changed in terms of coverage, but I ran into a technical problem and
> ran out of time for now to make that happen.  But you can still click
> your way through to the commit and see a coverage report for the
> commit diff.  In other words you can see which modified lines are run
> by make check-world, which seems quite useful.

Yes, it's definitely already useful. If you check
https://codecov.io/gh/postgresql-cfbot/postgresql/commits you can see
the code coverage for the various CF entries. Both the absolute coverage
and, more interestingly, the coverage of the changed lines. There's some
noticeable difference in coverage between the various entries...

E.g. very little of the new stuff in 
https://codecov.io/gh/postgresql-cfbot/postgresql/commit/ceaa3dbece3c9b98abcaa28009320fde45a83f88
is exercised.


> There are plenty more things we could stick into this pipeline, like
> LLVM sanitizer stuff, valgrind, Coverity, more compilers,  here>... but I'm not sure what things really make sense.  I may be
> placing undue importance on things that I personally screwed up
> recently :-D

A lot of these probably are too slow to be practical...  I know it's not
fun, but a good bit of that probably is going to be making the UI of the
overview a bit better. E.g. direct links to the build / tests logs from
travis/codecov/..., allowing to filter by failing to apply / failing
tests / ok, etc...


Some of this would be considerably easier if the project were ok with
having a .travis.yml in our sourcetree.

Regards,

Andres


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Thomas Munro]

Hi hackers,

 A couple of new experimental features on commitfest.cputube.org:

1.  I didn't have --enable-cassert enabled before.  Oops.
2.  It'll now dump a gdb backtrace for any core files found after a
check-world failure (if you can find your way to the build log...).
Thanks to Andres for the GDB scripting for this!
3.  It'll now push gcov results to codecov.io -- see link at top of
page.  Thanks again to Andres for this idea.
4.  It now builds a little bit faster due to -j4 (Travis CI VMs have 2
virtual cores) and .proverc -j3.  (So far one entry now fails in TAP
tests with that setting, will wait longer before drawing any
conclusions about that.)

The code coverage reports at codecov.io are supposed to allow
comparison between a branch and master so you can see exactly what
changed in terms of coverage, but I ran into a technical problem and
ran out of time for now to make that happen.  But you can still click
your way through to the commit and see a coverage report for the
commit diff.  In other words you can see which modified lines are run
by make check-world, which seems quite useful.

There are plenty more things we could stick into this pipeline, like
LLVM sanitizer stuff, valgrind, Coverity, more compilers, ... but I'm not sure what things really make sense.  I may be
placing undue importance on things that I personally screwed up
recently :-D

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Tom Lane]

Thomas Munro  writes:
> On Wed, Sep 13, 2017 at 2:34 AM, Alvaro Herrera  
> wrote:
>> Tom Lane wrote:
>>> Can you clarify what went wrong for you on that one?  I went to rebase it,
>>> but I end up with the identical patch except for a few line-numbering
>>> variations.

> It seems to be a legitimate complaint.  The rejected hunk is trying to
> replace this line:
> !   return exec_simple_check_node((Node *) ((ArrayCoerceExpr
> *) node)->arg);

> But you removed exec_simple_check_node in
> 00418c61244138bd8ac2de58076a1d0dd4f539f3, so this 02 patch needs to be
> rebased.

Hm.  My bad I guess --- apparently, the copy I had of this patch was
already rebased over that, but I'd not reposted it.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Thomas Munro]

On Tue, Sep 12, 2017 at 12:45 AM, Tomas Vondra
 wrote:
> That won't work until (2) is reliable enough. There are patches (for
> example my "multivariate MCV lists and histograms") which fails to apply
> only because the tool picks the wrong patch. Possibly because it does
> not recognize compressed patches, or something.

FWIW I told it how to handle your .patch.gz files and Alexander
Lakhin's .tar.bz2 files.  Your patch still didn't apply anyway due to
bitrot, but I see you've just posted a new one so it should hopefully
turn green soon.  (It can take a while because it rotates through the
submissions at a rate of one submission every 5 minutes after a new
commit to master is detected, since I don't want to get in trouble for
generating excessive load against the Commitfest, Github or (mainly)
Travis CI.  That's probably too cautious and over time we can revise
it.)

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Thomas Munro]

On Wed, Sep 13, 2017 at 2:34 AM, Alvaro Herrera  wrote:
> Tom Lane wrote:
>> Aleksander Alekseev  writes:
>> > I've ended up with this script [1]. It just generates a list of patches
>> > that are in "Needs Review" status but don't apply or don't compile. Here
>> > is the current list:
>>
>> > === Apply Failed: 29 ===
>> > https://commitfest.postgresql.org/14/1235/ (Support arrays over domain 
>> > types)
>>
>> Can you clarify what went wrong for you on that one?  I went to rebase it,
>> but I end up with the identical patch except for a few line-numbering
>> variations.
>
> I think "git apply" refuses to apply a patch if it doesn't apply
> exactly.  So you could use "git apply -3" (which merges) or just plain
> old "patch" and the patch would work fine.
>
> If the criteria is that strict, I think we should relax it a bit to
> avoid punting patches for pointless reasons.  IOW I think we should at
> least try "git apply -3".

The cfbot is not using git apply, it's using plain old GNU patch
invoked with "patch -p1".  From http://commitfest.cputube.org/ if you
click the "apply|failing" badge you can see the log from the failed
apply attempt.  It says:

== Fetched patches from message ID 3881.1502471872%40sss.pgh.pa.us
== Applying on top of commit 2d4a614e1ec34a746aca43d6a02aa3344dcf5fd4
== Applying patch 01-rationalize-coercion-APIs.patch...
== Applying patch 02-reimplement-ArrayCoerceExpr.patch...
1 out of 1 hunk FAILED -- saving rejects to file
src/pl/plpgsql/src/pl_exec.c.rej

It seems to be a legitimate complaint.  The rejected hunk is trying to
replace this line:

!   return exec_simple_check_node((Node *) ((ArrayCoerceExpr
*) node)->arg);

But you removed exec_simple_check_node in
00418c61244138bd8ac2de58076a1d0dd4f539f3, so this 02 patch needs to be
rebased.

> Also, at this point this should surely be just an experiment.

+1

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Andrew Dunstan]

On 09/12/2017 11:30 AM, Tom Lane wrote:
> Alvaro Herrera  writes:
>> Tom Lane wrote:
>>> Aleksander Alekseev  writes:
 === Apply Failed: 29 ===
 https://commitfest.postgresql.org/14/1235/ (Support arrays over domain 
 types)
>>> Can you clarify what went wrong for you on that one?  I went to rebase it,
>>> but I end up with the identical patch except for a few line-numbering
>>> variations.
>> I think "git apply" refuses to apply a patch if it doesn't apply
>> exactly.  So you could use "git apply -3" (which merges) or just plain
>> old "patch" and the patch would work fine.
>> If the criteria is that strict, I think we should relax it a bit to
>> avoid punting patches for pointless reasons.  IOW I think we should at
>> least try "git apply -3".
> FWIW, I always initially apply patches with good ol' patch(1).  So for
> me, whether that way works would be the most interesting thing.  Don't
> know about other committers' workflows.



Yeah, that's what I do, too.


>
>> Also, at this point this should surely be just an experiment.
> +1 ... seems like there's enough noise here that changing patch status
> based on the results might be premature.  Still, I applaud the effort.


I think a regular report of what doesn't apply and what doesn't build
will be very useful on its own, especially if there are links to the
failure reports. When we are satisfied that we're not getting
significant numbers of false negatives over a significant period we can
talk about automating CF state changes. I agree this is nice work.


cheers

andrew

-- 
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Peter Geoghegan]

On Tue, Sep 12, 2017 at 8:30 AM, Tom Lane  wrote:
> +1 ... seems like there's enough noise here that changing patch status
> based on the results might be premature.  Still, I applaud the effort.

There are always going to be cases where the tool fails, unless there
are more formal guidelines on patch submission. For example, if
someone posts multiple patches, and did not use git format-patch, then
the heuristic on which order to apply patches in will fail at times. I
don't think it's necessary to constrain the patch format that people
use too much, but this does need to be thought out.

Another thing that I think needs to happen is that the CF app needs to
add a web API. Friends of mine who actually know about this stuff tell
me that JSON API is a good default choice these days. Currently,
Thomas fetches information from the CF app using "screen scraping"
techniques, which are obviously fairly brittle. Similarly, Thomas'
patch testing web application should itself have a web API,
potentially usable by the CF app.

-- 
Peter Geoghegan


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Andres Freund]

Hi,

On 2017-09-12 11:30:33 -0400, Tom Lane wrote:
> One thing I'm a tad worried about is automatically running trojan-horsed
> submissions.  I hope the CI bot is tightly sandboxed.

Well, that's part of the nice thing here. The "really dangerous stuff"
is all running on a service that does so full-time, not on our
resources. Everyone can open git repos and open malicious PRs in them -
travis checks a *lot* of projects...   That's not to say your worries
are unfounded, just that they're not primarily ours. Although even the
patch file handling etc, seems worthy of a good bit of attention.

Greetings,

Andres Freund


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Tom Lane]

Alvaro Herrera  writes:
> Tom Lane wrote:
>> Aleksander Alekseev  writes:
>>> === Apply Failed: 29 ===
>>> https://commitfest.postgresql.org/14/1235/ (Support arrays over domain 
>>> types)

>> Can you clarify what went wrong for you on that one?  I went to rebase it,
>> but I end up with the identical patch except for a few line-numbering
>> variations.

> I think "git apply" refuses to apply a patch if it doesn't apply
> exactly.  So you could use "git apply -3" (which merges) or just plain
> old "patch" and the patch would work fine.

> If the criteria is that strict, I think we should relax it a bit to
> avoid punting patches for pointless reasons.  IOW I think we should at
> least try "git apply -3".

FWIW, I always initially apply patches with good ol' patch(1).  So for
me, whether that way works would be the most interesting thing.  Don't
know about other committers' workflows.

> Also, at this point this should surely be just an experiment.

+1 ... seems like there's enough noise here that changing patch status
based on the results might be premature.  Still, I applaud the effort.

One thing I'm a tad worried about is automatically running trojan-horsed
submissions.  I hope the CI bot is tightly sandboxed.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Alvaro Herrera]

Tom Lane wrote:
> Aleksander Alekseev  writes:
> > I've ended up with this script [1]. It just generates a list of patches
> > that are in "Needs Review" status but don't apply or don't compile. Here
> > is the current list:
> 
> > === Apply Failed: 29 ===
> > https://commitfest.postgresql.org/14/1235/ (Support arrays over domain 
> > types)
> 
> Can you clarify what went wrong for you on that one?  I went to rebase it,
> but I end up with the identical patch except for a few line-numbering
> variations.

I think "git apply" refuses to apply a patch if it doesn't apply
exactly.  So you could use "git apply -3" (which merges) or just plain
old "patch" and the patch would work fine.

If the criteria is that strict, I think we should relax it a bit to
avoid punting patches for pointless reasons.  IOW I think we should at
least try "git apply -3".

Also, at this point this should surely be just an experiment.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Tom Lane]

Aleksander Alekseev  writes:
> I've ended up with this script [1]. It just generates a list of patches
> that are in "Needs Review" status but don't apply or don't compile. Here
> is the current list:

> === Apply Failed: 29 ===
> https://commitfest.postgresql.org/14/1235/ (Support arrays over domain types)

Can you clarify what went wrong for you on that one?  I went to rebase it,
but I end up with the identical patch except for a few line-numbering
variations.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Thomas Munro]

On Tue, Sep 12, 2017 at 10:03 PM, Aleksander Alekseev
 wrote:
> Unless there are any objections I'm going to give these patches "Waiting
> on Author" status today (before doing this I'll re-run the script to
> make sure that the list is up-to-date). I'm also going to write one more
> email with CC to the authors of these patches to let them know that the
> status of their patch has changed.

I vote +1 with the caveat that you should investigate each one a bit
to make sure the cfbot isn't just confused about the patch first.
I've also been poking a few threads to ask for rebases + and report
build failures etc, though I haven't been changing statuses so far.

I like your idea of automating CF state changes, but I agree with
Tomas that the quality isn't high enough yet.  I think we should treat
this is a useful tool to guide humans for now, but start trying to
figure out how to integrate some kind of CI with the CF app.  It
probably involves some stricter rules about what exactly constitutes a
patch submission (acceptable formats, whether/how dependencies are
allowed etc).  Right now if cfbot fails to understand your patch
that's cfbot's fault, but if we were to nail down the acceptable
formats then it'd become your fault if it didn't understand your patch
:-D

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Aleksander Alekseev]

Hi Tomas,

> > I hope this observation will change your mind :)
> > 
> 
> Not sure. But I'm mostly just a passenger here, not the driver.

After working on this script for some time I got second thoughts
regarding this idea as well. The reason for this is that the script is
just a bunch of regular expressions that parse HTML. It works today but
doesn't look like something I'm willing to maintain in the long run.

I've ended up with this script [1]. It just generates a list of patches
that are in "Needs Review" status but don't apply or don't compile. Here
is the current list:

```
=== Apply Failed: 29 ===
https://commitfest.postgresql.org/14/1178/ (64-bit transaction identifiers)
https://commitfest.postgresql.org/14/1090/ (Add and report the new 
in_hot_standby GUC pseudo-variable.)
https://commitfest.postgresql.org/14/1139/ (allow mix of composite and scalar 
variables in target list)
https://commitfest.postgresql.org/14/1131/ (Allow users to specify multiple 
tables in VACUUM commands)
https://commitfest.postgresql.org/14/1284/ (Controlling toast_tuple_target to 
tune rows 2kB)
https://commitfest.postgresql.org/14/1001/ (Convert join OR clauses into UNION 
queries)
https://commitfest.postgresql.org/14/1272/ (faster partition pruning in planner)
https://commitfest.postgresql.org/14/1259/ (Fix race condition between SELECT 
and ALTER TABLE NO INHERIT)
https://commitfest.postgresql.org/14/1156/ (Gather speed-up)
https://commitfest.postgresql.org/14/1271/ (generated columns)
https://commitfest.postgresql.org/14/1059/ (Hash partitioning)
https://commitfest.postgresql.org/14/1138/ (Improve compactify_tuples and 
PageRepairFragmentation)
https://commitfest.postgresql.org/14/1136/ (Improve eval_const_expressions)
https://commitfest.postgresql.org/14/1124/ (Incremental sort)
https://commitfest.postgresql.org/14/1228/ (libpq: Support for Apple Secure 
Transport SSL library)
https://commitfest.postgresql.org/14/1238/ (multivariate MCV lists and 
histograms)
https://commitfest.postgresql.org/14/1202/ (New function for tsquery creation)
https://commitfest.postgresql.org/14/1250/ (Partition-wise aggregation/grouping)
https://commitfest.postgresql.org/14/1125/ (pg_basebackup has stricter 
tablespace rules than the server)
https://commitfest.postgresql.org/14/1183/ (Predicate locking in hash index)
https://commitfest.postgresql.org/14/1106/ (Range Merge Join)
https://commitfest.postgresql.org/14/1267/ (Retire polyphase merge)
https://commitfest.postgresql.org/14/1248/ (Subscription code improvements)
https://commitfest.postgresql.org/14/1235/ (Support arrays over domain types)
https://commitfest.postgresql.org/14/1148/ (Support 
target_session_attrs=read-only and eliminate the added round-trip to detect 
session status.)
https://commitfest.postgresql.org/14/1242/ (taking stdbool.h into use)
https://commitfest.postgresql.org/14/1130/ (Test coverage for FDW HANDLER DDL.)
https://commitfest.postgresql.org/14/1023/ (UPDATE of partition key)
https://commitfest.postgresql.org/14/775/ (Write Amplification Reduction Method 
(WARM))

=== Build Failed: 7 ===
https://commitfest.postgresql.org/14/528/ (Fix the optimization to skip 
WAL-logging on table created in same transaction)
https://commitfest.postgresql.org/14/1252/ (Foreign Key Arrays)
https://commitfest.postgresql.org/14/1062/ (Generic type subscripting)
https://commitfest.postgresql.org/14/962/ (new plpgsql extra_checks )
https://commitfest.postgresql.org/14/1113/ (Replication status in logical 
replication)
https://commitfest.postgresql.org/14/1260/ (Restricting maximum keep segments 
by repslots)
https://commitfest.postgresql.org/14/839/ (Temporal query processing with range 
types)
```

Unless there are any objections I'm going to give these patches "Waiting
on Author" status today (before doing this I'll re-run the script to
make sure that the list is up-to-date). I'm also going to write one more
email with CC to the authors of these patches to let them know that the
status of their patch has changed.

[1]: https://github.com/afiskon/py-autoreviewer

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Automatic testing of patches in commit fest

[Erik Rijkers]

On 2017-09-11 02:12, Thomas Munro wrote:

On Mon, Sep 11, 2017 at 11:40 AM, Michael Paquier
 wrote:

Thomas Munro has hacked up a prototype of application testing
automatically if patches submitted apply and build:
http://commitfest.cputube.org/


I should add: this is a spare-time effort, a work-in-progress and
building on top of a bunch of hairy web scraping, so it may take some
time to perfect.


It would be great if one of the intermediary products of this effort 
could be made available too, namely, a list of latest patches.


Or perhaps such a list should come out of the commitfest app.

For me, such a list would be even more useful than any subsequently 
processed results.


thanks,

Erik Rijkers



--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Tomas Vondra]

On 09/11/2017 03:01 PM, Aleksander Alekseev wrote:
> Hi Tomas,
> 
>>> Unless there are any objections to give this idea a try I'm willing to
>>> write and host a corresponding script.
>>>
>> That won't work until (2) is reliable enough. There are patches
>> (for example my "multivariate MCV lists and histograms") which
>> fails to apply only because the tool picks the wrong patch.
>> Possibly because it does not recognize compressed patches, or
>> something.
> 
> Agree. However we could simply add an "Enable autotests" checkbox to
> the commitfest application. In fact we could even left the
> commitfest application as is and provide corresponding checkboxes in
> the web interface of the "autoreviewer" application. Naturally every 
> automatically generated code review will include a link that
> disables autotests for this particular commitfest entry.
> 

I think we should try making it as reliable as possible first, and only
then consider adding some manual on/off switches. Otherwise the patches
may randomly flap between "OK" and "failed" after each new submission,
disrupting the CF process.

Making the testing reliable may even require establishing some sort of
policy (say, always send a complete patch, not just one piece).

>
> I hope this observation will change your mind :)
> 

Not sure. But I'm mostly just a passenger here, not the driver.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Aleksander Alekseev]

Hi Tomas,

> > Unless there are any objections to give this idea a try I'm willing to
> > write and host a corresponding script.
> > 
> That won't work until (2) is reliable enough. There are patches (for
> example my "multivariate MCV lists and histograms") which fails to apply
> only because the tool picks the wrong patch. Possibly because it does
> not recognize compressed patches, or something.

Agree. However we could simply add an "Enable autotests" checkbox to the
commitfest application. In fact we could even left the commitfest
application as is and provide corresponding checkboxes in the web
interface of the "autoreviewer" application. Naturally every
automatically generated code review will include a link that disables
autotests for this particular commitfest entry.

I hope this observation will change your mind :)

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Automatic testing of patches in commit fest

[Tomas Vondra]

On 09/11/2017 11:41 AM, Aleksander Alekseev wrote:
> Hi Thomas,
> 
> Great job!
> 

+1

> Here is a crazy idea. What if we write a script that would automatically
> return the patches that:
> 
> 1) Are not in "Waiting on Author" status
> 2) Don't apply OR don't pass `make installcheck-world`
> 
> ... to the "Waiting on Author" status and describe the problem through
> the "Add review" form on commitfest.postgresql.org? This will save a lot
> of time to the reviewers. Naturally nobody wants to spam pgsql-hackers@
> with automatic messages to often. I believe that sending such messages
> once a day would be enough.
> 
> Unless there are any objections to give this idea a try I'm willing to
> write and host a corresponding script.
> 

That won't work until (2) is reliable enough. There are patches (for
example my "multivariate MCV lists and histograms") which fails to apply
only because the tool picks the wrong patch. Possibly because it does
not recognize compressed patches, or something.

In such cases switching it to "Waiting on Author" automatically would be
damaging, as (a) there's nothing wrong with the patch, and (b) it's not
clear what to do to fix the problem.

So -1 to this for now, until we make this part smart enough.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Ashutosh Bapat]

On Mon, Sep 11, 2017 at 3:11 PM, Aleksander Alekseev
 wrote:
> Hi Thomas,
>
> Great job!
>
> Here is a crazy idea. What if we write a script that would automatically
> return the patches that:
>
> 1) Are not in "Waiting on Author" status
> 2) Don't apply OR don't pass `make installcheck-world`
>
> ... to the "Waiting on Author" status and describe the problem through
> the "Add review" form on commitfest.postgresql.org? This will save a lot
> of time to the reviewers. Naturally nobody wants to spam pgsql-hackers@
> with automatic messages to often. I believe that sending such messages
> once a day would be enough.
>
> Unless there are any objections to give this idea a try I'm willing to
> write and host a corresponding script.
>
+1 that would help.



-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Aleksander Alekseev]

Hi Thomas,

Great job!

Here is a crazy idea. What if we write a script that would automatically
return the patches that:

1) Are not in "Waiting on Author" status
2) Don't apply OR don't pass `make installcheck-world`

... to the "Waiting on Author" status and describe the problem through
the "Add review" form on commitfest.postgresql.org? This will save a lot
of time to the reviewers. Naturally nobody wants to spam pgsql-hackers@
with automatic messages to often. I believe that sending such messages
once a day would be enough.

Unless there are any objections to give this idea a try I'm willing to
write and host a corresponding script.

-- 
Best regards,
Aleksander Alekseev


signature.asc
Description: PGP signature

Re: [HACKERS] Automatic testing of patches in commit fest

[Thomas Munro]

On Mon, Sep 11, 2017 at 12:10 PM, Jeff Janes  wrote:
> On Sun, Sep 10, 2017 at 4:40 PM, Michael Paquier 
>> http://commitfest.cputube.org/
>
> This looks very interesting.  But when I click on a "build: failing" icon,
> it takes me to a generic page,  not one describing how that specific build
> is failing.

True.  The problem is that travis-ci.org doesn't seem to provide a URL
that can take you directly to the latest build for a given branch,
instead I'd need to figure out how to talk to their API to ask for the
build ID of the latest build for that branch.  For now you have to
note the Commitfest entry ID, and then when find the corresponding
branch (ie commitfest/14/1234) on the page it dumps you on.  It would
be nice to fix that.

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Thomas Munro]

On Mon, Sep 11, 2017 at 11:40 AM, Michael Paquier
 wrote:
> Thomas Munro has hacked up a prototype of application testing
> automatically if patches submitted apply and build:
> http://commitfest.cputube.org/
>
> I would recommend have a look at it from time to time if you are a
> patch author (or a reviewer) as any failure may say that your patch
> has rotten over time and needs a rebase. It is good to keep the commit
> fest entries build-clean at least for testers.

I should add: this is a spare-time effort, a work-in-progress and
building on top of a bunch of hairy web scraping, so it may take some
time to perfect.  One known problem at the moment is that the mail
archive (where patches are fetched from) cuts off long threads so it
doesn't manage to find the latest version of (for example) the
Partition-Wise Join patch.  Other problems include getting confused by
incidental material posted in .patch form, patches that depend on
other patches or patches whose apply order is not obvious, or ... etc
etc.  There are also a few other patches missing currently for various
reasons, which I intend to fix, gradually.

That said, it might already be useful for catching bitrot and things
like TAP test failures, contrib regressions and documentation build
errors which (based on the evidence I've seen so far) many submitters
aren't actually running themselves.  YMMV.

I've been having conversations with Magnus and Stephen about what a
more robust version integrated with the CF app might eventually look
like.  The basic idea here is:  Commitfest submissions are analogous
to pull request, which many comparable projects test automatically
using CI technology that is generously made available to open source
projects and well integrated with popular free git hosts.  We should
too!

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Automatic testing of patches in commit fest

[Jeff Janes]

On Sun, Sep 10, 2017 at 4:40 PM, Michael Paquier 
wrote:

> Hi all,
>
> Thomas Munro has hacked up a prototype of application testing
> automatically if patches submitted apply and build:
> http://commitfest.cputube.org/
>
> I would recommend have a look at it from time to time if you are a
> patch author (or a reviewer) as any failure may say that your patch
> has rotten over time and needs a rebase. It is good to keep the commit
> fest entries build-clean at least for testers.
> Thanks,


This looks very interesting.  But when I click on a "build: failing" icon,
it takes me to a generic page,  not one describing how that specific build
is failing.

Cheers,

Jeff

[HACKERS] Automatic testing of patches in commit fest

[Michael Paquier]

Hi all,

Thomas Munro has hacked up a prototype of application testing
automatically if patches submitted apply and build:
http://commitfest.cputube.org/

I would recommend have a look at it from time to time if you are a
patch author (or a reviewer) as any failure may say that your patch
has rotten over time and needs a rebase. It is good to keep the commit
fest entries build-clean at least for testers.
Thanks,
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers