Irksome indentation on Subtest line

[mattdeluco]

The # Subtest lines of test output are indented with four spaces:

For successful tests:

ok 15 - tests.test_account
    # Subtest: tests.test_split()
    ok 1 - Create split
    ok 2 - Create split 2
    1..2
ok 16 - tests.test_split

For failed tests:

    # Looks like you failed 7 test of 12
not ok 14 - tests.test_crud
# Failed test 14: "tests.test_crud"
    # Subtest: tests.test_account()
    ok 1 - No classifier entry

This is just a small sample, but in a huge list of tests I find it difficult to read. I constantly want to read the first left-aligned line as the header for the subsequent set of tests, but it's actually the next line indented by four spaces and preceded by #, and the line I'm reading is the summary for the set of tests above it.

Is this alignment intentional? I had a quick perusal of the TAP spec but didn't see anything for or against. I did find the Node Tap project with examples of output that is correctly aligned (IMO), and much easier to read.

What does everyone think of aligning the "# Subtest" line to the left edge rather than four spaces in?

I'd submit a PR, but I'm not sure which of the pgtap*.sql files in the sql/ directory to modify.

Pretty sure it's this line, though, that needs to have the spaces removed:
RETURN NEXT ' ' || diag_test_name('Subtest: ' || tests[i]);

[theory]

It's not intentional, it should indent every line with four spaces, thanks to the use of regexp_replace() here:

pgtap/sql/pgtap.sql.in

Lines 6393 to 6407 in 5bfd3c7

	-- Run the setup functions.
	FOR tap IN SELECT * FROM _runem(setup, false) LOOP
	RETURN NEXT regexp_replace(tap, '^', ' ', 'gn');
	END LOOP;
	-- Run the actual test function.
	FOR tap IN EXECUTE 'SELECT * FROM ' || tests[i] || '()' LOOP
	RETURN NEXT regexp_replace(tap, '^', ' ', 'gn');
	END LOOP;
	-- Run the teardown functions.
	FOR tap IN SELECT * FROM _runem(teardown, false) LOOP
	RETURN NEXT regexp_replace(tap, '^', ' ', 'gn');
	END LOOP;

But maybe it's not properly catching every line start?

[mattdeluco]

But are these lines:

not ok 14 - tests.test_crud
# Failed test 14: "tests.test_crud"

Actually part of setup, test, or teardown functions?

[theory]

Yeah, those bits are returned by the test functions. Gonna see if I can replicate this issue. What version are you using, BTW?

[theory]

Oh I see what you mean. Yes, this is intentional. Each test function is considered a single test. So you get either this for success:

    # Subtest: tests.test_crud()
    ok 1 - Crud
    ok 2 - More crud
    1..2
ok 14 - tests.test_crud

Or this:

    # Subtest: tests.test_crud()
    not ok 1 - Crud
    ok 2 - More crud
    1..2
    # Looks like you failed 1 test of 2
not ok 14 - tests.test_crud

So the result of the subtests are unindented on the last line. This style is copied directly from Test::More, which is the reference implementation for TAP and subtests.

I can see why you'd want the diagnostic output # Subtest: tests.test_crud() not to be indented, though. This might be a bit easier to read:

# Subtest: tests.test_crud()
    not ok 1 - Crud
    ok 2 - More crud
    1..2
    # Looks like you failed 1 test of 2
not ok 14 - tests.test_crud

[theory]

And I see that not indenting the diagnostic with the test name is exactly what node tap does.

The trick to changing it, however, would be that it would break any tests out there depending on Postgres testing, which diffs files. Suddenly folks would need to have an alternate expected output file with different indentation, which would be both annoying and not relevant to existing releases that have the old indentation and no control over the version of pgTAP that gets run. I need to give some thought as to whether there's a way to avoid that issue.

[theory]

Just pushed 9832e27 in branch unindent-subtest-diag to demonstrate the challenges with making this change for project that depend on make installcheck.

[mattdeluco]

Thank you for looking at this - I figured it might be a considerable change because of people / systems depending on the output being formatted in a particular fashion.

I'm on version 1.1.0 - sorry for the late reply, I've been on holiday with limited internet access.

[theory]

No worries. Just trying to get a sense of how much of an impact it would be. Not much discussion on the mail list.

[mattdeluco]

I wonder if I could figure out a way to colourize the output in my IDE or terminal, that would help with readability without requiring a breaking change.

[theory]

Well pg_prove has App::Prove's color output enabled by default, but I don't recall it it coloring diagnostics differently. Could be super useful, though.

[chipilov]

@theory Have you considered making this configurable via a backwards compatible change?

As far as I can tell from your experimental PR (9832e27), the change is in the _runner()function which is called directly by runtests(). Hence, you might be able to add a new argument at the end of the argument list for _runner() and the various runtests() overloads which indicates whether to add indentation or not. In order to be backwards compatible, the default value for the parameter would be to add the indentation.

The parameter could be boolean or some composite object which controls various formatting settings (the benefit of using a composite object would be that any similar changes in the future would NOT need a new parameter).

[theory]

Not really. See discussion here.