zig icon indicating copy to clipboard operation
zig copied to clipboard

Custom test runner

Open dweiller opened this issue 2 years ago • 2 comments

This PR adds the ability to specify a custom test runner via a new --test-runner CLI option, or the std.build.Builder.test_runner_path field. There are a few aspects I'm uncertain about:

  • A custom test runner probably does not want it's stdout to be consumed by the build runner so that the stdout of running zig build test-with-special-runner is the stdout of the custom test runner.
    • The stdout of .test kind LibObjExeStep is consumed by the build runner, but looking through the code, I wasn't sure that this was needed for .test kind steps.
    • This can be worked around by using a .test_exe kind LibObjExeStep and its run step, rather than a .test kind LibObjExeStep, in order for the stdout of the test to go to the stdout of the build runner, but I think it would be nicer to not have to do this to get the stdout of the test runner.
  • Does this need to be implemented for stage1? (The current implementation is only for stage2)
  • There's a basic standalone test, what other might be needed?
    • Is there a way to make a test that will invoke the build system from a subdirectory, (i.e. simulate cd /path/to/zig/test/standalone/test_runner_path/subdir && zig build test)?
  • Am I correct in assuming that nothing has to be added to the caching hash?

This should close #6621.

dweiller avatar Nov 02 '22 12:11 dweiller

Nice short and clean PR and good questions.

custom test runner probably does not want it's stdout to be consumed by the build runner

Why not? This allows conditional logic in the build runner (patch up of files, recompiling with added debug statements etc etc). If you write them to the build runner stdout, they are gone and you can not have them back.

Does this need to be implemented for stage1? (The current implementation is only for stage2)

Generally: If it is used for building stage2 (either in compiler code or in libstd), then yes. Otherwise, no. Not sure, what rule applies for code used for testing.

matu3ba avatar Nov 05 '22 15:11 matu3ba

custom test runner probably does not want it's stdout to be consumed by the build runner

Why not? This allows conditional logic in the build runner (patch up of files, recompiling with added debug statements etc etc). If you write them to the build runner stdout, they are gone and you can not have them back.

I think I was a little bit imprecise: I mean for the standard build runner shipped with Zig. Those sound like compelling use cases but the point is that by definition, the (standard) build runner can't know what the stdout of a custom test runner means. If you want the build system to do something with the output, you'll need a custom build runner anyway so the choice to capture the test runner's stdout could go in there rather than LibObjExeStep.

Does this need to be implemented for stage1? (The current implementation is only for stage2)

Generally: If it is used for building stage2 (either in compiler code or in libstd), then yes. Otherwise, no. Not sure, what rule applies for code used for testing.

Nothing here should be needed for building stage 2. My issue is that I don't really know c++, so it's difficult for me to make the equivalent changes to stage 1 (I have a commit for it, but it isn't quite right).

dweiller avatar Nov 05 '22 16:11 dweiller

  • A custom test runner probably does not want it's stdout to be consumed by the build runner so that the stdout of running zig build test-with-special-runner is the stdout of the custom test runner.

I'm not sure but if something is done it should probably be in a separate PR.

  • Does this need to be implemented for stage1? (The current implementation is only for stage2)

Only thing that should be done to stage1 is deleting it ASAP.

  • There's a basic standalone test, what other might be needed?

That is enough.

  • Is there a way to make a test that will invoke the build system from a subdirectory, (i.e. simulate cd /path/to/zig/test/standalone/test_runner_path/subdir && zig build test)?

That is what the standalone tests do?

  • Am I correct in assuming that nothing has to be added to the caching hash?

If omitting the flag can lead to different outcome then it should be cached. A custom test runner might use a different errors to skip tests or do something else so it should be cached.

Vexu avatar Nov 11 '22 19:11 Vexu

  • Is there a way to make a test that will invoke the build system from a subdirectory, (i.e. simulate cd /path/to/zig/test/standalone/test_runner_path/subdir && zig build test)?

That is what the standalone tests do?

I may have misunderstood how the standalone tests work - I thought they run zig build from the directory containing the specified build.zig. I was thinking about adding a test to check that path resolution for the test runner specified in a build.zig was correctly being done relative to the directory containing build.zig, rather than the directory zig build is invoked from (e.g. if you are in subdirectory), but maybe it's not necessary.

  • Am I correct in assuming that nothing has to be added to the caching hash?

If omitting the flag can lead to different outcome then it should be cached. A custom test runner might use a different errors to skip tests or do something else so it should be cached.

I haven't 100% understood how the caching system works, but I thought a custom test runner flag might not need to be added as using it would already change the root package of the compilation, so differences would be picked up by the part of the caching system that handles the root package (when addPackageTableToCacheHash() in Compilation.zig is called). If this is incorrect (or insufficient) I can easily add the path of the test runner to the cache as well.

dweiller avatar Nov 12 '22 03:11 dweiller

I may have misunderstood how the standalone tests work - I thought they run zig build from the directory containing the specified build.zig. I was thinking about adding a test to check that path resolution for the test runner specified in a build.zig was correctly being done relative to the directory containing build.zig, rather than the directory zig build is invoked from (e.g. if you are in subdirectory), but maybe it's not necessary.

This is handled by pathFromRoot but you should probably add a setter function that dupePaths it though.

I haven't 100% understood how the caching system works, but I thought a custom test runner flag might not need to be added as using it would already change the root package of the compilation, so differences would be picked up by the part of the caching system that handles the root package (when addPackageTableToCacheHash() in Compilation.zig is called). If this is incorrect (or insufficient) I can easily add the path of the test runner to the cache as well.

I tested this by defining a custom test runner with a different error for skipping and using --enable-cache and it correctly recompiled using the default test runner when omitting a flag so I guess nothing additional needs to be done.

Vexu avatar Nov 12 '22 11:11 Vexu

... but you should probably add a setter function that dupePaths it though.

Done. I think this is probably merge-ready then, unless I should rebase onto current master first.

dweiller avatar Nov 13 '22 02:11 dweiller