Time Nick Message 18:35 MTDiscord I have an idea to fix the benchmarks, but before I put it on the issue, someone tell me if this is worth considering. 18:36 MTDiscord I propose saving the output from the unit tests and diffing it with the output from the benchmarks to extract only the benchmark output. 18:37 MTDiscord Otherwise, the right way to solve the problem just in the context of Catch2 is probably to tag all the tests so we can specify which subset of tests to run. 18:38 MTDiscord The root problem is that the typical use-case for unit tests is that you compile them separately (individually usually) from your application and benchmarks, so Catch2 isn't designed well for what we do. 18:39 MTDiscord In retrospect this should have optimally come up before deciding to introduce Catch2 unit tests, but the way we discovered the problem existed was by running into it. 18:40 sfan5 we could definitely just move it into a separate executable 18:41 MTDiscord I imagined there was some reason we didn't do that already. Do we include tests and benchmarks in the binary for historical reasons? Does it have to do with being able to run tests on users platforms that we might not be able to test ourselves? 18:42 sfan5 you could call it historical reasons since the unittests were always in the main executable 18:42 sfan5 but actually 18:43 sfan5 for the benchmark run, can't we just disable building the unit tests? 18:43 MTDiscord Yes, totally. Is the benchmark run its own separate CI job? That makes the solution a piece of cake haha. 18:44 MTDiscord The job will use less time and resources as well if it doesn't have to compile 50 extra test files for nothing. 18:46 MTDiscord Also I see that #6987 happened; I don't see an issue suggesting splitting tests out of the main executable. Probably there are more important things to work on but that'd be in the same vein as not shipping devtest I think. 18:46 ShadowBot https://github.com/minetest/minetest/issues/6987 -- Stop shipping devtest game in release builds? 19:39 MTDiscord Indeed it is. Here's the cake: https://github.com/luanti-org/benchmarks/pull/7. I would've committed directly but it seems I lack the permissions on this repo. I suppose @celeron55 has to fix that. 19:39 MTDiscord Yep, I think benchmarks and tests in separate executables would be nice, but aren't very important. 19:42 MTDiscord FWIW, I don't think the way it went is too bad. The "breakage" that happened here was not easy to expect and is of low significance. I think it's fine if something like this only gets caught later. 19:43 MTDiscord Indeed, but I like retrospectives. 19:43 MTDiscord Next time I set something like that up I will be aware of this potential problem so I learned something valuable. 19:47 MTDiscord Fixed 19:47 MTDiscord ...maybe? 19:47 MTDiscord Are you in the Engine team? 19:50 MTDiscord Yes you are, so it's fixed now 19:51 MTDiscord yep, just merged. hope it works. (this kind of CI is awkward to test) 19:51 MTDiscord not like there is anything to break 19:54 MTDiscord i suppose we'll see whether it worked next time we push to luanti 19:55 MTDiscord ah yeah @celeron55, while you're here, maybe irc.minetest.net -> irc.luanti.org? 19:56 MTDiscord should probably do that at the same time as the IRC channel rename, if we do that 19:58 celeron55 yeah i definitely should try to get the irc channel rename done just about yesterday but the fact is i haven't