Re: CI and test improvements - Peter Eisentraut

public inbox for [email protected]  
help / color / mirror / Atom feed

From: Peter Eisentraut <[email protected]>
To: Justin Pryzby <[email protected]>
Cc: Thomas Munro <[email protected]>
Cc: Tom Lane <[email protected]>
Cc: Andres Freund <[email protected]>
Cc: Andrew Dunstan <[email protected]>
Cc: [email protected]
Cc: Noah Misch <[email protected]>
Cc: Michael Paquier <[email protected]>
Cc: Anastasia Lubennikova <[email protected]>
Cc: Robert Haas <[email protected]>
Cc: Melanie Plageman <[email protected]>
Cc: Daniel Gustafsson <[email protected]>
Cc: samay sharma <[email protected]>
Subject: Re: CI and test improvements
Date: Wed, 15 Mar 2023 10:58:41 +0100
Message-ID: <[email protected]> (raw)
In-Reply-To: <ZA/[email protected]>
References: <[email protected]>
	<[email protected]>
	<[email protected]>
	<[email protected]>
	<CA+hUKGL=z6-kU80nPTjWqV=Xo=mpM36Lca=ngTr4LqcCf=tHzQ@mail.gmail.com>
	<CA+hUKG+kFpXfCsJQPJApkk7WrBFYYSao0V8q+cj88KQBV8XYRQ@mail.gmail.com>
	<[email protected]>
	<CA+hUKGLNyuvxTcchJe+DK_=_+fZTYCKA4c4o8eDwO5OYc2=qXQ@mail.gmail.com>
	<[email protected]>
	<[email protected]>
	<ZA/[email protected]>

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/[email protected];.

 > [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.

view thread (4+ messages)  latest in thread

reply

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Reply to all the recipients using the --to and --cc options:
  reply via email

  To: [email protected]
  Cc: [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected], [email protected]
  Subject: Re: CI and test improvements
  In-Reply-To: <[email protected]>

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox