Re: CI and test improvements - Mailing list pgsql-hackers

On 14.03.23 05:56, Justin Pryzby wrote:
> I'm soliticing feedback on those patches that I've sent recently - I've
> elided patches if they have some unresolved issue.

 > [PATCH 1/8] cirrus/windows: add compiler_warnings_script

Needs a better description of what it actually does.  (And fewer "I'm 
not sure how to write this ..." comments ;-) )  It looks like it would 
fail the build if there is a compiler warning in the Windows VS task? 
Shouldn't that be done in the CompilerWarnings task?

Also, I see a bunch of warnings in the current output from that task. 
These should be cleaned up in any case before we can let a thing like 
this loose.

(The warnings are all like

C:\python\Include\pyconfig.h(117): warning C4005: 'MS_WIN64': macro 
redefinition

so possibly a single fix can address them all.)


 > [PATCH 2/8] cirrus/freebsd: run with more CPUs+RAM and do not repartition

I don't know enough about this.  Maybe Andres or Thomas want to take 
this.  No concerns if it's safe.


 > [PATCH 3/8] cirrus/freebsd: define 
ENFORCE_REGRESSION_TEST_NAME_RESTRICTIONS

Looks sensible.


 > [PATCH 4/8] pg_upgrade: tap test: exercise --link and --clone

I haven't been able to get any changes to the test run times outside of 
noise from this.  But some more coverage is sensible in any case.

I'm concerned that with this change, the only platform that tests --copy 
is Windows, but Windows has a separate code path for copy.  So we should 
leave one Unix platform to test --copy.  Maybe have FreeBSD test --link 
and macOS test --clone and leave the others with --copy?


 > [PATCH 5/8] WIP: ci/meson: allow showing only failed tests ..

>>> 7e09035f588 WIP: ci/meson: allow showing only failed tests ..
>>
>> I'm not sure I like this one.  I sometimes look up the logs of non-failed
>> tests to compare them with failed tests, to get context to could lead to
>> failures.  Maybe we can make this behavior adjustable. But I've not been
>> bothered by the current behavior.
> 
> It's adjustable by un/setting the environment variable.
> 
> I'm surprised to hear that anyone using cirrusci (with or without cfbot)
> wouldn't prefer the behavior this patch implements.  It's annoying to
> search find the logs for the (typically exactly one) failing test in
> cirrus' directory of 200some test artifacts.  We're also uploading a lot
> of logs for every failure.  (But I suppose this might break cfbot's new
> client side parsing of things like build logs...)

One thing that actually annoys me that is that a successful run does not 
upload any test artifacts at all.  So, I guess I'm just of a different 
opinion here.


 > [PATCH 6/8] cirrus: code coverage

This adds -Db_coverage=true to the FreeBSD task.  This has a significant 
impact on the build time.  (+50% at least, it appears.)

I'm not sure the approach here makes sense.  For example, if you add a 
new test, the set of changed files is just that test.  So you won't get 
any report on what coverage change the test has caused.

Also, I don't think I trust the numbers from the meson coverage stuff 
yet.  See for example 
<https://www.postgresql.org/message-id/Y/3AI+/MqKcjLk/T@paquier.xyz>.


 > [PATCH 7/8] cirrus: upload changed html docs as artifacts
 > [PATCH 8/8] +html index file

This builds the docs twice and then analyzes the differences between the 
two builds.  This also affects the build times quite significantly.

How useful is this actually?  People who want to look at the docs can 
build them locally.  There are no platform dependencies or anything like 
that where having them built elsewhere is of advantage.