Обсуждение: Minor meson gripe
Currently, meson has a test suite named "setup". According to the Wiki, this is needed to get something equivalent to "make check", by running "meson test -v --suite setup --suite regress". Some questions about this: * Isn't it confusing that we have a suite by that name, given that we also need to use the unrelated --setup flag for some nearby testing recipes? * Why do we actually need a "setup" suite? Offhand it appears that a simple "meson test -v --suite regress" works just as well. Have I missed something? -- Peter Geoghegan
Hi, On 2023-02-09 11:01:31 -0800, Peter Geoghegan wrote: > Currently, meson has a test suite named "setup". According to the > Wiki, this is needed to get something equivalent to "make check", by > running "meson test -v --suite setup --suite regress". Yep. > Some questions about this: > > * Isn't it confusing that we have a suite by that name, given that we > also need to use the unrelated --setup flag for some nearby testing > recipes? Hm. I don't find it particularly confusing, but I don't think I'm a good judge of that, too close. > * Why do we actually need a "setup" suite? > > Offhand it appears that a simple "meson test -v --suite regress" works > just as well. Have I missed something? It'll work, but only if you have run setup before. And it'll not use changed C code. The setup suite creates the installation in tmp_install/. So if you haven't run the tests before, it'll fail due to that missing. If you have run it before, but have changed code, it'll not get used. The background for the issue is that while meson test supports dependencies for each test, and will build exactly the required dependencies if you run individual tests with meson test, it unfortunately also adds all the test dependencies to the default ninja target. That's mostly for historical reasons, because initially meson didn't support dependencies for tests. There's recent work on changing that though. Creating the temp installation every time you run 'ninja' would not be nice. On slower machines it can take quite a while. I think medium term we should just stop requiring a temporary install to run tests, it's substantial, unnecessary, overhead, and it requires us to build way too much to run basic tests. It'd not take a whole lot to make that work: - a search path for finding extensions, which'd be very useful for other reasons as well - a way to tell 'postgres', 'initdb' etc, which use find_other_exec(), that they should use PATH - a way to tell initdb where to find things like postgres.bki, postgres where it can find timezone data, etc. Greetings, Andres Freund
On Thu, Feb 9, 2023 at 12:56 PM Andres Freund <andres@anarazel.de> wrote: > > * Isn't it confusing that we have a suite by that name, given that we > > also need to use the unrelated --setup flag for some nearby testing > > recipes? > > Hm. I don't find it particularly confusing, but I don't think I'm a good judge > of that, too close. > It'll work, but only if you have run setup before. And it'll not use changed C > code. I see. It's not that confusing on its own, but it does cause confusion once you consider how things fit together. Suppose I want to do the equivalent of running the amcheck tests -- the tests that run when "make check" runs from contrib/amcheck with an autoconf build. That looks like this with our current meson workflow: meson test -v --suite setup --suite amcheck Now consider what I have to run to get the equivalent of a "make installcheck" run from the contrib/amcheck directory: meson test -v --setup running --suite amcheck-running Notice that I have to specify "--suite setup" in the first example, whereas I have to specify "--setup running" in the second example instead -- at the same point in. Also notice the rest of the details almost match. This makes it quite natural to wonder if "--suite setup" is related to "--setup running" in some way, which is not the case at all. They're two wholly unrelated concepts. Why not change the suite name to tmp_install? That immediately reminds me of what's really going on here, since I'm used to seeing that directory name. And it clashes with "--suite setup" in a way that seems useful. -- Peter Geoghegan
Hi, On 2023-02-09 15:34:34 -0800, Peter Geoghegan wrote: > Why not change the suite name to tmp_install? That immediately reminds > me of what's really going on here, since I'm used to seeing that > directory name. And it clashes with "--suite setup" in a way that > seems useful. The individual test is actually named tmp_install. I thought it might be useful to add further things to the setup "stage", hence the more general name. I e.g. have a not-quite-done patch that creates a "template initdb", which pg_regress and tap tests automatically use (except if non-default options are required), which quite noticably reduces test times (*). But something needs to create the template initdb, and initdb doesn't run without an installation, so we need to run it after the temp_install. Of course we could wrap that into one "test", but it seemed nicer to see if you fail during installation, or during initdb. So for that I added a separate test, that is also part of the setup suite. Of course we could still name the suite tmp_install (or to be consistent with the confusing make naming, have a temp-install target, which creates the tmp_install directory :)). I guess that'd still be less confusing? I'm not at all wedded to the "setup" name. Greetings, Andres Freund * approximate test time improvements: local: 688.67user 154.44system 1:08.29elapsed 1234%CPU (0avgtext+0avgdata 138984maxresident)k -> 172.37user 109.43system 1:00.12elapsed 468%CPU (0avgtext+0avgdata 139168maxresident)k The 4x reduction in CPU cycles is pretty bonkers. To bad wall clock time doesn't improve that much - we end up waiting for isolationtester, pg_upgrade tests to finish. CI freebsd: 06:30 -> 05:00 CI linux, sanitize-address: 5:40->4:30 CI linux, sanitize-undefined,alignment: 3:00->2:15 CI windows 12:20 -> 10:00 CI macos: 10:20 -> 08:00 I expect it to very substantially speed up the valgrind builfarm animal. By far the most cycles are spent below initdb. https://github.com/anarazel/postgres/tree/initdb-caching
On Thu, Feb 9, 2023 at 4:33 PM Andres Freund <andres@anarazel.de> wrote: > The individual test is actually named tmp_install. I thought it might be > useful to add further things to the setup "stage", hence the more general > name. I did notice that, but only after sitting with my initial confusion for a while. > I e.g. have a not-quite-done patch that creates a "template initdb", which > pg_regress and tap tests automatically use (except if non-default options are > required), which quite noticably reduces test times (*). But something needs to > create the template initdb, and initdb doesn't run without an installation, so > we need to run it after the temp_install. > > Of course we could wrap that into one "test", but it seemed nicer to see if > you fail during installation, or during initdb. So for that I added a separate > test, that is also part of the setup suite. But what are the chances that the setup / tmp_install "test" will actually fail? It's almost a test in name only. > Of course we could still name the suite tmp_install (or to be consistent with > the confusing make naming, have a temp-install target, which creates the > tmp_install directory :)). I guess that'd still be less confusing? Yes, that definitely seems like an improvement. I don't care about the tiny inconsistency that this creates. I wonder if this could be addressed by adding another custom test setup, like --setup running, used whenever you want to just run one or two tests against an ad-hoc temporary installation? Offhand it seems as if add_test_setup() could support that requirement? -- Peter Geoghegan
Hi, On 2023-02-09 17:00:48 -0800, Peter Geoghegan wrote: > On Thu, Feb 9, 2023 at 4:33 PM Andres Freund <andres@anarazel.de> wrote: > > I e.g. have a not-quite-done patch that creates a "template initdb", which > > pg_regress and tap tests automatically use (except if non-default options are > > required), which quite noticably reduces test times (*). But something needs to > > create the template initdb, and initdb doesn't run without an installation, so > > we need to run it after the temp_install. > > > > Of course we could wrap that into one "test", but it seemed nicer to see if > > you fail during installation, or during initdb. So for that I added a separate > > test, that is also part of the setup suite. > > But what are the chances that the setup / tmp_install "test" will > actually fail? It's almost a test in name only. I've seen more failures than I'd like. Permission errors, conflicting names, and similar things. But mainly that was a reference to running initdb, which I've broken temporarily many a time. > > Of course we could still name the suite tmp_install (or to be consistent with > > the confusing make naming, have a temp-install target, which creates the > > tmp_install directory :)). I guess that'd still be less confusing? > > Yes, that definitely seems like an improvement. I don't care about the > tiny inconsistency that this creates. Then lets do that - feel free to push something, or send something for review. Otherwise I'll try to get to it, but I owe a few people work before this... > I wonder if this could be addressed by adding another custom test > setup, like --setup running, used whenever you want to just run one or > two tests against an ad-hoc temporary installation? Offhand it seems > as if add_test_setup() could support that requirement? What precisely do you mean with "ad-hoc temporary installation"? I was wondering about adding a different setup that'd use the "real" installation to run tests. But perhaps that's something different than what you have in mind? The only restriction I see wrt add_test_setup() is that it's not entirely trivial to use a "runtime-variable" path to an installation. Greetings, Andres Freund
On Thu, Feb 9, 2023 at 5:17 PM Andres Freund <andres@anarazel.de> wrote: > I've seen more failures than I'd like. Permission errors, conflicting names, > and similar things. But mainly that was a reference to running initdb, which > I've broken temporarily many a time. We've all temporarily broken initdb literally thousands of times, I'm sure. > > > Of course we could still name the suite tmp_install (or to be consistent with > > > the confusing make naming, have a temp-install target, which creates the > > > tmp_install directory :)). I guess that'd still be less confusing? > > > > Yes, that definitely seems like an improvement. I don't care about the > > tiny inconsistency that this creates. > > Then lets do that - feel free to push something, or send something for > review. Otherwise I'll try to get to it, but I owe a few people work before > this... I'll try to get to it soon. Note that I've been adding new stuff to the meson Wiki page, in the hope of saving other people the trouble of figuring some of these details out for themselves. You might want to take a look at that at some point. > > I wonder if this could be addressed by adding another custom test > > setup, like --setup running, used whenever you want to just run one or > > two tests against an ad-hoc temporary installation? Offhand it seems > > as if add_test_setup() could support that requirement? > > What precisely do you mean with "ad-hoc temporary installation"? I mean "what make check does". > I was wondering about adding a different setup that'd use the "real" > installation to run tests. But perhaps that's something different than what > you have in mind? I wasn't thinking about the installation. Actually, I was, but the thought went no further than "I wish I didn't have to think about the installation". I liked that autoconf had "make check" and "make installcheck" variants that worked in a low context way. It's great that "meson test" runs all of the tests very quickly -- that should be maintained, even at some cost elsewhere. And it would be nice to do away with the tmp_install thing. But as long as we have it, it would be nice to make the way that we run a subset of test suites against a running server similar to the way that we run a subset of test suites against a throwaway installation (ala "make check"). > The only restriction I see wrt add_test_setup() is that it's not entirely > trivial to use a "runtime-variable" path to an installation. I personally have no problem with that, though of course I could have easily overlooked something. -- Peter Geoghegan