Re: [RFC/PATCH] Makefile: add test-all target

On Fri, Dec 10 2021, Jeff King wrote:

On Fri, Dec 10, 2021 at 03:38:53AM +0100, Ævar Arnfjörð Bjarmason wrote:

quoted

I just don't think it makes any sense that I edit say refs.[ch], run
"make test" locally, but only see that something broke in scalar's
specific use of libgit.a later when I look at GitHub CI.

I'm definitely sympathetic to this. Having been surprised by CI failure
on something that worked locally is annoying at best, and downright
frustrating when you can't easily reproduce the problem.

But isn't that already true for most of the value that CI provides?
While part of its purpose may be a back-stop for folks who don't run
"make test" locally, I think the biggest value is that it covers a much
wider variety of platforms and scenarios that you don't get out of "make
test" already.

In some of those cases you can reproduce the problem locally by tweaking
build or test knobs. But in others it can be quite a bit more
challenging (e.g., something that segfaults only on Windows). At least
in the proposed change here you'd only be a "make test-all" away from
reproducing the problem locally.

I dunno. I don't feel that strongly either way about whether scalar
tests should be part of "make test". Mostly just observing that this is
not exactly a new case.

Yes. I'm not saying that "make test" should always run what a full CI
run covers.

Just that a proposed change that's really only adding one-more-test-file
testing a thing in contrib in the sense that we test
t/t9902-completion.sh should similarly be part of "make test".

quoted

If I'm preparing patches for submission I'll need to get CI passing, so
I'll need to fix those tests & behavior either way as it's
in-tree. Knowing about the failures later-not-sooner wastes more time,
not less.

I think there's probably a tradeoff here. How often you get a "late"
notification of a bug (and how much of your time that wastes) versus how
much time you spend locally running tests that you don't care about.

I do agree that CI presents a bit of a conundrum for stuff at the edge
of the project. It's become a de facto requirement for it to pass. In
general that's good. But it means that features which were introduced
under the notion of "the people who care about this area will tend to
its maintenance" slowly become _everybody's_ problem as soon as they
have any CI coverage. Another example here is the cmake stuff. Or the
recent discussion about "-x" and bash.

I wonder if there's a good way to make some CI results informational,
rather than "failing". I.e., run scalar tests via CI, but if you're not
working on scalar, you don't have to care. Folks who are interested in
the area would keep tabs on those results and make sure that Junio's
tree stays passing.

I think if we're not caring about its failures in combination with
git.git changes there wouldn't be much point in having it in-tree at
all. That would just be like what we've got with git-cinnabar.git.

I would like it in tree. I just don' think the test/CI setup needs to be
a special snowflake.

That view disagrees with the final paragraph here, though:

quoted

The reason we do that with the completion is because some changes to
e.g. tweak getopts will need to have a corresponding change to the
completion.

The reason we've not done that with contrib/{subtree,mw-to-git}/ is
because those are thoroughly in the category of only incidentally being
in-tree.
[...]
Scalar is thoroughly on the "completion" side of that divide, not
"subtree".

I haven't followed the discussion closely, but in my mind "scalar" was
still in the "it may live in-tree for convenience, but people who aren't
working on it don't necessarily need to care about it" camp. Maybe
that's not the plan, though.

Since v1 of the series[1] it's been compiled unconditionally, and there
have been tests. We just didn't run the tests.

In v6 the tests started being run as part of CI, which was ejected in
v10 due to "[an] unrelated patch series does not interact well with
them", which as I noted upthread in [2] isn't accurate, so I think the
stated reason for ejecting the CI from the proposed topic doesn't
reflect reality.

Since then 1d855a6b335 (Merge branch 'ab/ci-updates' into next,
2021-12-07) landed, so I'd think that any narrow tweaks to get the CI
working could be based on top of that topic.

1. https://lore.kernel.org/git/pull.1005.git.1630359290.gitgitgadget@gmail.com/ (local)
2. https://lore.kernel.org/git/211207.86ilw0matb.gmgdl@evledraar.gmail.com/ (local)