Thread: [HACKERS] Automatic testing of patches in commit fest
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
On Sun, Sep 10, 2017 at 4:40 PM, Michael Paquier <michael.paquier@gmail.com> 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
On Mon, Sep 11, 2017 at 11:40 AM, Michael Paquier <michael.paquier@gmail.com> 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
On Mon, Sep 11, 2017 at 12:10 PM, Jeff Janes <jeff.janes@gmail.com> wrote: > On Sun, Sep 10, 2017 at 4:40 PM, Michael Paquier <michael.paquier@gmail.com> >> 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
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
On Mon, Sep 11, 2017 at 3:11 PM, Aleksander Alekseev <a.alekseev@postgrespro.ru> 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
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
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
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
On 2017-09-11 02:12, Thomas Munro wrote: > On Mon, Sep 11, 2017 at 11:40 AM, Michael Paquier > <michael.paquier@gmail.com> 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
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 todetect 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
On Tue, Sep 12, 2017 at 10:03 PM, Aleksander Alekseev <a.alekseev@postgrespro.ru> 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
Aleksander Alekseev <a.alekseev@postgrespro.ru> 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
Tom Lane wrote: > Aleksander Alekseev <a.alekseev@postgrespro.ru> 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 Herrera https://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
Alvaro Herrera <alvherre@alvh.no-ip.org> writes: > Tom Lane wrote: >> Aleksander Alekseev <a.alekseev@postgrespro.ru> 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
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
On Tue, Sep 12, 2017 at 8:30 AM, Tom Lane <tgl@sss.pgh.pa.us> 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
On 09/12/2017 11:30 AM, Tom Lane wrote: > Alvaro Herrera <alvherre@alvh.no-ip.org> writes: >> Tom Lane wrote: >>> Aleksander Alekseev <a.alekseev@postgrespro.ru> 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 Dunstan https://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
On Wed, Sep 13, 2017 at 2:34 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > Tom Lane wrote: >> Aleksander Alekseev <a.alekseev@postgrespro.ru> 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
On Tue, Sep 12, 2017 at 12:45 AM, Tomas Vondra <tomas.vondra@2ndquadrant.com> 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
Thomas Munro <thomas.munro@enterprisedb.com> writes:
> On Wed, Sep 13, 2017 at 2:34 AM, Alvaro Herrera <alvherre@alvh.no-ip.org> 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
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, <your idea 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 -- 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
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, <your idea > 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
On Mon, Sep 18, 2017 at 2:39 PM, Andres Freund <andres@anarazel.de> 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
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, <your idea here> 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