Default Formatter Implementation - followup

[fpeterschmitt]

Based on https://github.com/gavv/httpexpect/pull/78

• Uses *Context everywhere instead of Reporter. However this introduces breaking changes.
• Changes from #78 comments.

@gavv @goku321

[fpeterschmitt]

I don't think what i came with is good:

• The Formatter has a dependency on Context and Reporter. I think it should be the opposite, where the Reporter may want to have a Formatter, and the Reporter takes a Context as parameter, as well as a Failure when applicable. This allows for complete control on how messages are formatted and reported. In our case, we will want the default reporter to have a default formatter, and have coloured formatters as well. But that might be undesirable when reporting into a file for example.
• The Reporter could probably be more than "simply" a layer to make tests fail (as it is currently). In fact, the Success/Failure methods of the Formatter could be implemented at the Reporter level. This would allow for example implementations of Reporters that would write results in files to be exploited in dashboards… and whatnot :) Note: i'm not saying that httpexpect should implement any "external" reporting ability, and the current Reporter with assertions will not be more complicated than what it is with such change.
• In this PR, the Context has a ref to a Request, and the Request has a ref to the Context. Not sure what to do about that and if it is even avoidable.

I don't think we can do something better regarding formatters and reporters without breaking some public API though.

[fpeterschmitt]

Another thing: the Failure struct contains the assertionName and two values.

But that doesn't always apply: for example the "NotEmpty" or "Item" assertions/helpers cannot really return an expected value.

So either the assertionName field contain values that we define in const so formatter/reporter can switch case on them, or we make Failure an interface and each assertion/helper will then use a concrete type.

type Failure interface {
    error                           // have a default formatting in case a formatter/reporter doesn't support explicitly a failure type
    HTTPExpectFailure() // enforce type checking
}

type FailureBase struct {
    Expected interface{} // to be used when applicable
    Actual      interface{}
}

func (f FailureBase) HTTPExpectFailure() {}

type NotEmptyFailure struct {
    FailureBase
}

func (f NotEmptyFailure) Error() string {
    return fmt.Sprintf("expected not empty value")
}

[fpeterschmitt]

Or then expected of Failure can contain predefined values:

var ExpectedEmpty = Value{...}
var ExpectedNotEmpty = Value{…}
…

probably way simpler :smile: but doesn't work for "expected length"

[fpeterschmitt]

@gavv gentle ping ^^

[gavv]

@Leryan Thanks a lot for PR, much appreciated! I was hoping to review it on this week, but it seems I'll have a chance only on the next one, sorry.

[fpeterschmitt]

@Leryan Thanks a lot for PR, much appreciated! I was hoping to review it on this week, but it seems I'll have a chance only on the next one, sorry.

no worries, i'll have plenty of time in the next months to come, no pressure anyway :blush:

[gavv]

@Leryan

Sorry again for late reply and thanks for looking into this!

I think I got your point. In current PR, we have Formatter which does two things: defines how to handle various control flow events (assertion started, ended, succeeded, failed), and also defines how to format failure into a string. And it doesn't look good to mix these two responsibilities.

On the other hand, we have Reporter, which is basically anything with Errorf(). And we also have LoggerReporter which is anything with Errorf() and Logf(). And LoggerReporter is a subset if testing.T and you can pass it to httpexpect, or use custom implementation.

To avoid mixing responsibilities in Formatter, you're suggesting to move the first of them (handling flow events) to Reporter. However, it will break API and will remove the nice feature when you can say "I don't care on all these reporters, loggers, and formatters" and just pass testing.T as reporter and skip any other configuration.

So maybe divide responsibilities in other way?

For example, we can split Formatter into two entities:

• the first one is doing only one thing: formatting; we may keep the name Formatter for it, but it will have a simpler interface (maybe only one method Format, or FormatFailure)

• the second one handles control flow events; it will have the same interface as currently Formatter has in this PR; the default implementation will do real work only in Failure() method: it will call Formatter.FormatFailure() to convert failure into a string, and then call Reporter.Errrof() to report it to the user; we may name it, say, AssertionHandler, or something similar

• when you implement custom AssertionHandler, you can use default formatter and reporter if you wish, or you can completely ignore any of them and do something else

What do you think?

In this PR, the Context has a ref to a Request, and the Request has a ref to the Context. Not sure what to do about that and if it is even avoidable.

I think this is OK since it actually reflects what we want. Request refers to context because it's part of the hierarchy. Context refers to request because it refers to all important members of the hierarchy (request, response, failure).

@gavv about not breaking the public API, i think there is a problem with the NewNumber NewObject (and so on…) constructors: since we need to pass the context instead, that will break any code that relies on those functions.

Even if we break API in other places, I'd like to keep as much compatibility as possible.

In this specific case, all these NewFoo() methods allow to construct a "standalone" (i.e. without using Expect and Config structs) value and do some assertions.

I think each NewFoo() constructor can keep its signature and just construct a new Context from scratch using provided Reporter. On the other hand, when you call Expect.Foo(), like Expect.Array(), the returned object should inherit Context from Expect.

To achieve this, we will likely need two constructors: exported NewFoo(Reporter) that creates new context and unexported newFoo(Context) which inherits context from parent.

So either the assertionName field contain values that we define in const so formatter/reporter can switch case on them, or we make Failure an interface and each assertion/helper will then use a concrete type.

I'd prefer the first approach: just add flags defining the kind of failure (does it have two values, expected and actual, or only one value, etc.), and maybe some other hints how to format it.

Why: 1) Failure is a kind of DTO object, so it would be nice to keep things simple and make it "plain" struct; 2) This approach will allow us to easily add more formatting hints (flags) in future, if we need them.

[fpeterschmitt]

For example, we can split Formatter into two entities:

* the first one is doing only one thing: formatting; we may keep the name Formatter for it, but it will have a simpler interface (maybe only one method Format, or FormatFailure)

* the second one handles control flow events; it will have the same interface as currently Formatter has in this PR; the default implementation will do real work only in Failure() method: it will call Formatter.FormatFailure() to convert failure into a string, and then call Reporter.Errrof() to report it to the user; we may name it, say, AssertionHandler, or something similar

* when you implement custom AssertionHandler, you can use default formatter and reporter if you wish, or you can completely ignore any of them and do something else

What do you think?

Great! I didn't take the time to formulate correctly what i thought but yeah this is totally the idea.

@gavv about not breaking the public API, i think there is a problem with the NewNumber NewObject (and so on…) constructors: since we need to pass the context instead, that will break any code that relies on those functions.

Even if we break API in other places, I'd like to keep as much compatibility as possible.

In this specific case, all these NewFoo() methods allow to construct a "standalone" (i.e. without using Expect and Config structs) value and do some assertions.

I think each NewFoo() constructor can keep its signature and just construct a new Context from scratch using provided Reporter. On the other hand, when you call Expect.Foo(), like Expect.Array(), the returned object should inherit Context from Expect.

To achieve this, we will likely need two constructors: exported NewFoo(Reporter) that creates new context and unexported newFoo(Context) which inherits context from parent.

:+1:

So either the assertionName field contain values that we define in const so formatter/reporter can switch case on them, or we make Failure an interface and each assertion/helper will then use a concrete type.

I'd prefer the first approach: just add flags defining the kind of failure (does it have two values, expected and actual, or only one value, etc.), and maybe some other hints how to format it.

Why: 1) Failure is a kind of DTO object, so it would be nice to keep things simple and make it "plain" struct; 2) This approach will allow us to easily add more formatting hints (flags) in future, if we need them.

:+1:

Thanks for your feedback, i'll work on this very soon :)

[fpeterschmitt]

@gavv hey there :) i've pushed a substantial amount of changes, i think this is close to what you want.

Also, i used a bit of a trickery with the Reporter interface in order to avoid the creation of unexposed constructors, all while keeping the public API compatible.

Currently, the DefaultFormatter doesn't format anything really, but i'd like your review before continuing :)

Thanks!

Edit: this is how it looks like with a simple test case:

❭ go test -v ./...
=== RUN   TestWikipedia
=== RUN   TestWikipedia/Success_Test_Case
    printer.go:54: GET https://www.wikipedia.org/
=== RUN   TestWikipedia/Failing_Test_Case
    printer.go:54: GET https://www.wikipedia.org/
    reporter.go:23: 
        	Error Trace:	reporter.go:23
        	            				assertionhandler.go:80
        	            				assertionhandler.go:84
        	            				chain.go:31
        	            				string.go:121
        	            				string.go:96
        	            				main_test.go:20
        	Error:      	
        	            	assertion: string
        	            	expected:  ""
        	Test:       	TestWikipedia/Failing_Test_Case
--- FAIL: TestWikipedia (0.78s)
    --- PASS: TestWikipedia/Success_Test_Case (0.62s)
    --- FAIL: TestWikipedia/Failing_Test_Case (0.17s)
FAIL
FAIL	herw	0.786s
FAIL

The code:

package httpexpect_realworld_test

import (
	"testing"

	"github.com/gavv/httpexpect/v2"
)

func TestWikipedia(t *testing.T) {
	makeT := func(t *testing.T) *httpexpect.Expect {
		return httpexpect.New(t, "https://www.wikipedia.org/")
	}
	t.Run("Success Test Case", func(t *testing.T) {
		e := makeT(t)
		e.GET("/").Expect().Body().NotEmpty()
	})

	t.Run("Failing Test Case", func(t *testing.T) {
		e := makeT(t)
		e.GET("/").Expect().Body().Empty()
	})
}

[gavv]

Impressive work! I've started reviewing it.

[gavv]

Please let me know if you would like to leave some issues unresolved in this PR and postpone them to future PR(s). It's ok since we're merging into a feature branch.

[fpeterschmitt]

Please let me know if you would like to leave some issues unresolved in this PR and postpone them to future PR(s). It's ok since we're merging into a feature branch.

Your call, you're the reviewer and i'm fine with either solution :) If that makes your life easier then let's :)

[fpeterschmitt]

hello @gavv

could you ensure that the context branch is in sync with master so i can update my branch as well?

thanks :)

[gavv]

Your call, you're the reviewer and i'm fine with either solution :) If that makes your life easier then let's :)

Yeah, I'm also fine with both. Let's go with a single pr then.

could you ensure that the context branch is in sync with master so i can update my branch as well?

Done!

(I've rebased context on master and force-pushed it)

[Heidern]

@fpeterschmitt thanks for your work on this. I'm trying to implement a new formatter (using your fork / branch,) but httpexpect.Failure properties are not exposed. That means I'm stuck with the default message formats for now.

[fpeterschmitt]

@fpeterschmitt thanks for your work on this. I'm trying to implement a new formatter (using your fork / branch,) but httpexpect.Failure properties are not exposed. That means I'm stuck with the default message formats for now.

Seems legit :sweat_smile: I just pushed the changes, it should be all you need :)

[Heidern]

@fpeterschmitt awesome, that works. thanks! btw, any idea of when your changes will make their way into the main branch?

[fpeterschmitt]

@Heidern as i am not the maintainer of httpexpect i cannot tell :)

However, the PR isn't ready, as the default formatter should be at least equivalent to the current display way.

I do not have time currently to continue working on this PR sadly :'(

[gavv]

Hi,

I've finished this changeset in #150.

@fpeterschmitt @goku321 thanks a lot for you work and constructive discussion!! I've squashed your commits into two (d8cbc2ff45e72040fc89553f9280c7cc51ff0b1a and 2f0d6874b6050a245085d9cc7fa22716b5017cdf) and added third commit with remaining changes (8ff72952c8b9ee632cba628faebfb33562ab15a5).

I created new PR #150 with those commit and will close previous PRs.

Some things that I've changed:

• Reporter is not deprecated; AssertionHandler does not implement Reporter; instead, AssertionHandler works on top of Reporter and Formatter, and chain works on top of AssertionHandler; the user may provide either Reporter or AssertionHandler, depending on their needs.

• AssertionContext does not contain AssertionHandler or Formatter or Reporter. Instead:

  • chain contains AssertionHandler and AssertionContext
  • AssertionHandler contains Formatter and Reporter

• LoggerReporterNamer renamed to TestingTB. New(LoggerReporter) is deprecated and replaced with Default(TestingTB). The idea with casting Reporter to Namer dynamically is abandoned, test name will work only if you use Default or explicitly specify it in Config.

• chain now dynamically tracks path to current assertion using enter(), leave(), and clone() methods; assertion path replaces assertion name and is now part of assertion context instead of assertion failure.

• I switched to approach with private constructors, as discussed above. Everything is now always created using those constructors, which allows to ensure that chain is inherited properly (using clone() method). It made the code much more cleaner and also allowed to remove contextReporterWrapper.

• Reworked list of assertion types.

• Renames to unify assertion-related types (AssertionType, AssertionContext, AssertionFailure, AssertionHandler).

• Other minor improvements.

[gavv]

Closing with respect to #150.