Strict mode for testexec structure #12

TripleDogDare · 2022-11-14T18:51:21Z

Adds a strict mode such that the testexec structure is enforced by default.

Original:

Adds tests to output that will show up as skipped. This should make the behavior of testexec easier understand and debug using go test -v.
Adds a RecursionFn to testexec to give users the ability to control recursion.
Adds a Path variable to DirEnt structs which contains its full path.

Overall default behavior is unchanged.

warpfork

keeping Path tracked: 👍
lots of additional logging: 👍
tests tests tests: 👍 🙇
introducing publicly exported callbacks to the recursion process: 👎 adds complexity and maintenance surface, and yet I have a hard time seeing how it'll adds value.

warpfork · 2022-11-14T19:46:55Z

testexec/testexec.go

 }

 func (tcfg Tester) doSequence(t *testing.T, hunk *testmark.Hunk, stdin io.Reader, stdout, stderr io.Writer) (exitcode int) {
 	t.Helper()
+	t.Logf("running sequence: %q", hunk.Name)


Strikes me as usually redundant due to the presence of the test names?

eh kind of. Assuming you know where you started from and don't care to know whether it's running a sequence or a script without looking at the md.

testexec/testexec.go

testexec/defaults.go

warpfork · 2022-11-14T21:09:51Z

(is this now a soft prerequisite for what we want #11 to become? Because of the Path keeping bits?)

TripleDogDare · 2022-11-14T22:38:48Z

(is this now a soft prerequisite for what we want #11 to become? Because of the Path keeping bits?)

No. I've been using it to debug which blobs are running and which aren't. I would also use it to make some changes to how /net/ works in warpforge. Might be useful in #11 somehow but I haven't really thought about that at all and isn't the motivation for it at the moment.

Needing to keep Path is just part of having an API that I can work with for writing tests for what things are and aren't called which are legible. Without the full path I don't have a way to distinguish subdirectories of the same name with different parentage. Here's the alternative ways I've thought of to accomplish that:

Add a parent *DirEnt to DirEnt.
Make a breaking change to the APIs to take DirEnt by reference instead of by value.
Excavating the call stack.
Make the RecursionFn include path information as a parameter and just keep passing that along. This would look similar to fs.WalkDir.

TripleDogDare · 2022-11-15T00:18:08Z

I'm trying to actually use it for the /net/ stuff now and maaaaybe that's not going to work the way I hoped.

warpfork · 2022-11-15T00:21:06Z

Yeah I don't entirely love the "/net/" pattern that we ended up with in warpforge, but I don't hate it either. And I think generally some labelling info on the uppermost parts of the hunk paths seemed to be sufficient in all the usecases I've thought of so far.

TripleDogDare · 2022-11-15T00:47:45Z

It would be nice to be able to have it anywhere but it gets a bit wonky where I need to skip the /net/ node itself and recurse into children. But that won't work without additional changes. Although it seems like we have /net/sequence as a pattern so it should work actually.

However, using the existing code for /net/ is really confusing where it always skips non-net code with the same parent node.

$ go test -v -run TestExecFixtures ./cmd/warpforge/ | rg 'sequence|script' | rg -v 'special leaf'

List of executable hunks in the markdown file.

    main_test.go:57: base-workspace/script
    main_test.go:57: base-workspace/then-generatehtml/sequence
    main_test.go:57: base-workspace/then-runmodule/sequence
    main_test.go:57: catalog-git/net/sequence
    main_test.go:57: catalog-git/net/then-check/script
    main_test.go:57: catalog/net/then-add-tar/sequence
    main_test.go:57: catalog/net/then-add-tar/then-check/script
    main_test.go:57: catalog/sequence
    main_test.go:57: catalog/then-add/then-bundle/sequence
    main_test.go:57: catalog/then-add/then-ferk-with-plot/sequence
    main_test.go:57: catalog/then-add/then-ferk/sequence
    main_test.go:57: catalog/then-ls/sequence
    main_test.go:57: catalog/then-update/then-quickstart/sequence
    main_test.go:57: catalog/then-update/then-quickstart/then-run/sequence
    main_test.go:57: checkformula/sequence
    main_test.go:57: checkmodule/sequence
    main_test.go:57: checkplot/sequence
    main_test.go:57: runall/sequence
    main_test.go:57: runformula/net/sequence

List of hunks that were executed

    main_test.go:82: running sequence: "runall/sequence"
    main_test.go:82: running sequence: "checkformula/sequence"
    main_test.go:82: running sequence: "checkmodule/sequence"
    main_test.go:82: running sequence: "checkplot/sequence"
    main_test.go:82: running sequence: "runformula/net/sequence"
    main_test.go:82: running script: "base-workspace/script"
    testexec.go:338: running sequence: "base-workspace/then-runmodule/sequence"
    testexec.go:338: running sequence: "base-workspace/then-generatehtml/sequence"
    main_test.go:82: dir "catalog/net" does not contain a 'script' or 'sequence' hunk and will be skipped
    main_test.go:82: running sequence: "catalog-git/net/sequence"
    testexec.go:338: running script: "catalog-git/net/then-check/script"

TripleDogDare · 2022-11-15T00:59:00Z

If I take out the existing /net/ code and change the recursion function to

func recursionFn(t *testing.T, dir testmark.DirEnt) error {
	if dir.Name == "net" && *testutil.FlagOffline {
		return fmt.Errorf("skipping test %q due to offline flag: %w", dir.Path, testexec.SkipRecursion)
	}
	return testexec.RecursionFn_Then(t, dir)
}

Then I get a different set of results that will need a bit of tweaking in the markdown to get the desired effect. It should behave more consistently for mixed net and non-net groups. But there needs to be a thing/net/sequence AND a thing/sequence for it to work.

$ go test -v -run TestExecFixtures ./cmd/warpforge/ | rg 'sequence|script' | rg -v 'special leaf'
    ...
    main_test.go:82: running sequence: "runall/sequence"
    main_test.go:82: running sequence: "checkformula/sequence"
    main_test.go:82: running sequence: "checkmodule/sequence"
    main_test.go:82: running sequence: "checkplot/sequence"
    main_test.go:82: dir "runformula" does not contain a 'script' or 'sequence' hunk and will be skipped
    main_test.go:82: running script: "base-workspace/script"
    testexec.go:338: running sequence: "base-workspace/then-runmodule/sequence"
    testexec.go:338: running sequence: "base-workspace/then-generatehtml/sequence"
    main_test.go:82: running sequence: "catalog/sequence"
    testexec.go:338: running sequence: "catalog/then-ls/sequence"
    testexec.go:338: dir "catalog/then-add" does not contain a 'script' or 'sequence' hunk and will be skipped
    testexec.go:338: dir "catalog/then-update" does not contain a 'script' or 'sequence' hunk and will be skipped
    main_test.go:82: dir "catalog-git" does not contain a 'script' or 'sequence' hunk and will be skipped

warpfork

I'm a lot happier with this now, with this more specific focus 🙇

testexec/testexec.go

- Adds tests to output that will show up as skipped. This should make the behavior of testexec easier understand and debug using go test -v. - Adds a RecursionFn to testexec to give users the ability to control recursion. - Adds a Path variable to DirEnt structs which contains its full path. Overall default behavior is unchanged.

- Removes the recursion override. - Adds strict default mode - Adds tests for failing conditions that need to be run manually because there's really just not a good way to do that. - Splits test exercise markdown files into self, strict, and invalid based on whether they should pass or fail always or in strict mode.

warpfork · 2022-11-27T01:20:20Z

testexec/testexec.go

+		if tcfg.DisableStrictMode {
+			t.SkipNow()
+		}
+		t.FailNow()


Hm, pretty minor thing, but on re-review I notice these four lines recur a number of times pretty much verbatim. I think extracting a tiny skipOrFailStrictly(t) function might be worth it.

The reason to log for it could even be moved in as a fmt string arg and varargs, although I'm neutral on whether or not it's done that way.

Adding the logs seems good.

warpfork reviewed Nov 14, 2022

View reviewed changes

TripleDogDare changed the title ~~Add recursion flexibility~~ Strict mode for testexec structure Nov 16, 2022

warpfork reviewed Nov 16, 2022

View reviewed changes

testexec/testexec.go Outdated Show resolved Hide resolved

testexec/testexec.go Outdated Show resolved Hide resolved

TripleDogDare force-pushed the recursion-control branch from 809162f to 0080f43 Compare November 26, 2022 22:48

TripleDogDare force-pushed the recursion-control branch 2 times, most recently from 8832a5d to a9ca5f6 Compare November 27, 2022 00:35

TripleDogDare force-pushed the recursion-control branch from a9ca5f6 to 59d311a Compare November 27, 2022 00:38

warpfork reviewed Nov 27, 2022

View reviewed changes

add skipOrFailStrictlyf function

7855604

TripleDogDare force-pushed the recursion-control branch from d438295 to 7855604 Compare November 27, 2022 01:37

warpfork merged commit fc3ad54 into warpfork:master Nov 27, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strict mode for testexec structure #12

Strict mode for testexec structure #12

TripleDogDare commented Nov 14, 2022 •

edited

Loading

warpfork left a comment

warpfork Nov 14, 2022

TripleDogDare Nov 14, 2022

warpfork commented Nov 14, 2022

TripleDogDare commented Nov 14, 2022

TripleDogDare commented Nov 15, 2022

warpfork commented Nov 15, 2022

TripleDogDare commented Nov 15, 2022 •

edited

Loading

TripleDogDare commented Nov 15, 2022 •

edited

Loading

warpfork left a comment

warpfork Nov 27, 2022

TripleDogDare Nov 27, 2022

Strict mode for testexec structure #12

Strict mode for testexec structure #12

Conversation

TripleDogDare commented Nov 14, 2022 • edited Loading

warpfork left a comment

Choose a reason for hiding this comment

warpfork Nov 14, 2022

Choose a reason for hiding this comment

TripleDogDare Nov 14, 2022

Choose a reason for hiding this comment

warpfork commented Nov 14, 2022

TripleDogDare commented Nov 14, 2022

TripleDogDare commented Nov 15, 2022

warpfork commented Nov 15, 2022

TripleDogDare commented Nov 15, 2022 • edited Loading

TripleDogDare commented Nov 15, 2022 • edited Loading

warpfork left a comment

Choose a reason for hiding this comment

warpfork Nov 27, 2022

Choose a reason for hiding this comment

TripleDogDare Nov 27, 2022

Choose a reason for hiding this comment

TripleDogDare commented Nov 14, 2022 •

edited

Loading

TripleDogDare commented Nov 15, 2022 •

edited

Loading

TripleDogDare commented Nov 15, 2022 •

edited

Loading