Skip to content

Make assertion constructors return errors instead of panicking #56

Description

@korya

Rearchitecture step 3 of 6. Sequence: #54#553#48#18#41.

Problem

Assertion constructors can fail, but their signature has no way to say so, so they panic:

func AssertBodyMatch(expPattern string) Assertion {
	re := regexp.MustCompile(expPattern)   // panics on user input
	// …
}

Three constructors compile user-supplied regexes this way — AssertHeaderMatch (assertions.go:81), AssertBodyMatch (assertions.go:125), AssertRedirectMatch. A typo in a CI pipeline produces a raw Go stack trace and exit code 2.

#17 is the symptom; this is the cause. #17 could be closed by wrapping the three call sites in parseAssertionFlags, but the design would still be "a constructor that can fail has no error return", and the next fallible assertion added will reintroduce it.

Suggested fix

func AssertBodyMatch(expPattern string) (Assertion, error) {
	re, err := regexp.Compile(expPattern)
	if err != nil {
		return nil, fmt.Errorf("--assert-body: %w", err)
	}
	// …
}

parseAssertionFlags collects errors and reports them as invalid arguments instead of crashing:

$ http-assert --assert-body '[unclosed' https://example.com
Error: Invalid value for --assert-body flag: error parsing regexp: missing closing ]: `[unclosed`
[exit=103]

Second change, same file: give assertions an identity

While the signatures are being touched, Assertion should stop being an opaque func(*httpResponse) error. Today a failure can only ever be a string, which is why structured output (#45) can't be added cheaply — there is nothing to serialize.

Before:

type Assertion func(res *httpResponse) error

After:

type Assertion interface {
	Kind() string                    // "status", "header", "body", "redirect"
	Check(res *httpResponse) *Failure
}

type Failure struct {
	Kind     string
	Target   string  // header name, or "" 
	Expected any
	Actual   any
}

The functional style is genuinely good and worth preserving where it can be — this is about the result carrying structure, not about abandoning composability.

Why this is step 3

It should follow #55 (extract run()), because error-returning constructors need somewhere to return to — with os.Exit still buried in die(), the errors have no path to the surface. It should precede #48 so the end-to-end tests assert against the fixed exit codes rather than being written twice.

Related

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions