wire icon indicating copy to clipboard operation
wire copied to clipboard

wire: Load has no tests

Open zombiezen opened this issue 7 years ago • 3 comments
trafficstars

The wire.Load function, used by Wire's interactive commands, does not have any tests. While it shares much of the code with the well-tested Generate function, it has enough additional functionality that it should probably have its own test suite.

zombiezen avatar Oct 30 '18 15:10 zombiezen

My proposal is to: -- Move the code that implements wire show (using Load but then doing a bunch of processing on top of it) from cmd/wire/main.go into wire.go. The command-line tool should only do trivial formatting of the results. -- Add a call to this new function into wire_test.go, and record/replay the result (in some trivially-formatted way) in a golden file for each test in the existing Wire test suite.

This way, we get improved coverage for the non-trivial code in the cmd package (which currently has 0% coverage) as well as for Load.

I discussed this with @zombiezen , who was unsure if he agreed and wanted to think about it more.

More context for this discussion:

IIUC, @zombiezen's vision is that the wire package should expose the results of its processing so that other libraries can use them and build on them. That's why Load is exported, and exposes a bunch of non-trivial wire data structures.

My intuition is that the wire package should not export those kinds of things; e.g., Load should not be exposed. It should have a narrow set of things that it exposes, which are exactly the things that the cmd package needs for the cmd line application, and which should only require simple processing to be rendered as output. The suggestion above for show is an example.

-- Advantage: the public interface for the wire package is much much smaller. -- Advantage: testing is much easier, as we can test the much smaller exposed API for each existing wire test. -- Disadvantage: New commands for the command line tool, or new tools, require changes to the wire package itself to support them.

IMHO the advantages are big, and the disadvantage is OK -- we should ask for such features as contributions to wire anyway. People can always fork the repo if they really want to get into the innards.

vangent avatar Nov 16 '18 20:11 vangent

I think we can still add tests for wire.Load without changing the existing structure of the commands. #12 tracks potentially making the internal/wire package import-able; let's discuss structural changes there.

I think the larger issue here is determining how to express "correct" output for Load. I suspect we would need to do some design work and more critically think about what's important to test for.

zombiezen avatar Dec 06 '18 17:12 zombiezen

OK let's decide on #12 before moving ahead here then, since I think the right way to test Load depends on whether we expose it publicly.

vangent avatar Dec 06 '18 17:12 vangent