DONT MERGE YET - REVIEW COMMENTS PLEASE - Test renovation - in preparation for fixing the Hook handler bug and adding other features. #659

Johnlon · 2024-10-30T20:12:22Z

🤔 What's changed?

suite_context_test.go is now testing solely using the Public of godog api.
To highlight it's new lease of life it is renamed to functional_tests.go

Previously this suite wasn't really testing godog at all, because it was testing a fake godog-like mock up.
Consequently a bunch of the tests weren't actually doing anything useful.
The most useful tests therefore were the fmt_output_tests which DID go via the real product.

Much of the code changes are a consequence of fixing these issues.

Fixed broken tests
Removed duplicates tests
Removed bogus / useless tests
Relocated tests
Added new tests

The public api has an additiive change TestSuite.RunWithResults() that returns the exit code plus a reference to the storage.
This addiitonal public api is really useful for various reasons (eg postprocessing) but importantly its a hook needed to do some of the tests without totally hackery.

Formatter.Close()

This is conceivably a breaking if anyone has written their own formatter.

I added Close() to the Formatter api so that the framework will reliably close any files opened inside the formatters (eg cuke).

This gap was the cause of a testing problem that became apparent once using the proper cuke public api because the formatter output files were not getting flushed because there were never getting closed during the tests. The close is gives predictable (and correct) behaviour.
This problem would only afflict godog where it's used in the "Library" mode because users of the CLI would have has the files closed as the CLI exited.

⚡️ What's your motivation?

Product is a bit too hard to work with and I want to add some features/fixes.
This is only a partial renovation.

🏷️ What kind of change is this?

🏦 Refactoring/debt/DX (improvement to code design, tooling, etc. without changing behaviour)
🐛 Bug fix (non-breaking change which fixes a defect)
⚡ New feature (non-breaking change which adds new behaviour)

♻️ Anything particular you want feedback on?

📋 Checklist:

I agree to respect and uphold the Cucumber Community Code of Conduct
I've changed the behaviour of the code
- I have added/updated tests to cover my changes.
My change requires a change to the documentation.
- I have updated the documentation accordingly.
Users should know about my change
- [] I have added an entry to the "Unreleased" section of the CHANGELOG, linking to this pull request.

This text was originally generated from a template, then edited by hand. You can modify the template here.

…"features" and it's massive set of steps into run_test - removed some duplicate tests from example_subtests_test.go

Previously the suite_context_tests weren't actually testing godog at all but an exploded version of what the internal code might have looked like at some point in the past. Removed some junk test cases around events.feature that weren't actually testing godog but some irrelevant stuff in the exploded code.

…s on it or do whatever the end user wants make honnef scan whole tree

github-actions · 2024-10-30T20:12:52Z

Go API Changes

# github.com/cucumber/godog
## incompatible changes
NewBaseFmt: changed from func(string, io.Writer) *github.com/cucumber/godog/internal/formatters.Base to func(string, io.WriteCloser) *github.com/cucumber/godog/internal/formatters.Base
NewCukeFmt: changed from func(string, io.Writer) *github.com/cucumber/godog/internal/formatters.Cuke to func(string, io.WriteCloser) *github.com/cucumber/godog/internal/formatters.Cuke
NewEventsFmt: changed from func(string, io.Writer) *github.com/cucumber/godog/internal/formatters.Events to func(string, io.WriteCloser) *github.com/cucumber/godog/internal/formatters.Events
NewJUnitFmt: changed from func(string, io.Writer) *github.com/cucumber/godog/internal/formatters.JUnit to func(string, io.WriteCloser) *github.com/cucumber/godog/internal/formatters.JUnit
NewPrettyFmt: changed from func(string, io.Writer) *github.com/cucumber/godog/internal/formatters.Pretty to func(string, io.WriteCloser) *github.com/cucumber/godog/internal/formatters.Pretty
NewProgressFmt: changed from func(string, io.Writer) *github.com/cucumber/godog/internal/formatters.Progress to func(string, io.WriteCloser) *github.com/cucumber/godog/internal/formatters.Progress
## compatible changes
Attach: added
Attachment: added
Attachments: added
ErrAmbiguous: added
ExitFailure: added
ExitOptionError: added
ExitSuccess: added
NopCloser: added
RunResult: added
StepAmbiguous: added
TestSuite.RunWithResult: added

# github.com/cucumber/godog/colors
## incompatible changes
Colored: changed from func(io.Writer) io.Writer to func(io.WriteCloser) io.WriteCloser
Uncolored: changed from func(io.Writer) io.Writer to func(io.WriteCloser) io.WriteCloser

# github.com/cucumber/godog/formatters
## incompatible changes
Formatter.Ambiguous: added
Formatter.Close: added
FormatterFunc: changed from func(string, io.Writer) Formatter to func(string, io.WriteCloser) Formatter

# summary
Inferred base version: v0.14.1
Suggested version: v0.15.0

go run ./cmd/godog -f progress -c 4 --strict .. because thats what github does

noodnik2 · 2024-10-31T03:24:24Z

functional_test.go

+/*
+This file contains some functional tests for the godog library.
+This is the glue code that can be run against features/*feature
+and those feature files are written to use godog features to test godod.


godod (sp)

noodnik2 · 2024-10-31T03:28:16Z

functional_test.go

+The general pattern is the feature files have top level or "outer" steps with
+that carry mini-features in docstrings and also expected outputs
+also in docstrings.
+The glue code for the "inner" features is separated below into "inner" and "outer" steps.


noodnik2 · 2024-10-31T03:42:22Z

internal/utils/utils.go

@@ -19,3 +20,100 @@ func S(n int) string {
 var TimeNowFunc = func() time.Time {
 	return time.Now()
 }
+


mega-nit:

Consider creating a new file for your new formatter utilities - e.g., formatters.go

Adding a new set of tools - especially if they focus on a single, new concern - into something as abstract a utils.go is a pet peeve of mine.

noodnik2 · 2024-10-31T03:45:04Z

run.go

+	return multiFmt.FormatterFunc(suiteName, output), nil
+}
+
+func configureMultiFormatter(opt Options, output io.WriteCloser) (multiFmt ifmt.MultiFormatter, err error) {


nit: Consider dropping use of named return values (cost/benefit)?

noodnik2

First, thanks for the effort to counter entropy - always a good & worthy battle if the heart is in the right place, and I "feel" it is here from what I've seen. 😊

I've sprinkled in a few "first impression" (mostly nit type) comments without having really grokked the changes, which were just too many to take in at once.

Consider breaking this up into (maybe prioritized) sub-concerns? I'd be more likely to be interested & able to give you better feedback & generate more excitement for pulling downstream. 🤓

nhv96 · 2024-10-31T11:04:54Z

internal/formatters/fmt_multi.go

+// TestRunStarted triggers TestRunStarted for all added formatters.
+func (r repeater) Close() error {
+	var err error
+	for _, f := range r {
+		e := f.Close()
+		if e != nil {
+			err = e
+		}
+	}
+	return err
+}
+


You forgot to update the comment. Also do we need to add test for this method?

Will see what the existing test strat is for this and look to improve.

Johnlon · 2024-10-31T12:31:22Z

First, thanks for the effort to counter entropy - always a good & worthy battle if the heart is in the right place, and I "feel" it is here from what I've seen. 😊

I've sprinkled in a few "first impression" (mostly nit type) comments without having really grokked the changes, which were just too many to take in at once.

Consider breaking this up into (maybe prioritized) sub-concerns? I'd be more likely to be interested & able to give you better feedback & generate more excitement for pulling downstream. 🤓

I think this PR is already the atom of improvement.
I'd prefer to get it merged and then add the fixes for the bugs that motivated this

nhv96 · 2024-10-31T12:47:14Z

run.go

+	Name                 string
+	TestSuiteInitializer func(*TestSuiteContext)
+	ScenarioInitializer  func(*ScenarioContext)
+	Options              *Options // TODO mutable value - is this necessary?


With your refactoring and move the function body to one upper level, I can only think the reason for us to keep using pointer here is to avoid incompatible with older version 🤔
Looks like they wanted to separate the concerns, test suite initialization vs runner logic to run that suite, so changes to the options configuration within the function body of runner should not reflected to the test suite.

yes I suspect you are right - how tolerant should we be of breaking changes.
I had undone some work to avoid breaks in this pr.

I'd say we can tolerant to the point that user's expectations and use cases of godog library still remain the same. Except for the hook ordering bug we're going to fix, I really don't see we're going to "break" anything else.

nhv96 · 2024-11-01T07:30:11Z

internal/formatters/fmt_base.go

+func NewBase(suite string, out io.WriteCloser) *Base {
 	return &Base{
 		suiteName: suite,
 		indent:    2,


Regarding this approach, do you think it will force the users (who use custom fmt) to working too much on their end to adapt to our changes? I think having a Close() method is a great enhancement, but I'm considering the case that someone is having problem with hook ordering bug, and expecting the new release will fix it, but then in order to use it they need to update their custom code first...

Should we use another way such as saying "we're going to fix this, and also we're migrating the Formatter API to a new version, you don't need to update anything yet and still have new features to use"? I have the following code that seems to work on my local testing

Suggested change

func NewBase(suite string, out io.WriteCloser) *Base {

return &Base{

suiteName: suite,

indent: 2,

type baseAdapter struct {

out io.Writer

}

func (f baseAdapter) Write(p []byte) (n int, err error) {

return f.out.Write(p)

}

func (f baseAdapter) Close() error {

return nil

}

// NewBase creates a new base formatter.

func NewBase(suite string, out io.Writer) *Base {

var wc io.WriteCloser

w, ok := out.(io.WriteCloser)

if ok {

wc = w

} else {

wc = baseAdapter{out: out}

}

return &Base{

suiteName: suite,

indent: 2,

out: wc,

Lock: new(sync.Mutex),

}

}

// Base is a base formatter.

type Base struct {

suiteName string

out io.WriteCloser

indent int

Storage *storage.Storage

Lock *sync.Mutex

}

This way we can still migrate to use io.WriteCloser in our internal logic, but user don't need to know or don't need to update anything right away.
Ps: sorry my english is not that good so my reasoning is kinda long.

Johnlon added 11 commits October 24, 2024 02:46

- moved all the tests from suite_context_test.go that do NOT rely on …

bacc276

…"features" and it's massive set of steps into run_test - removed some duplicate tests from example_subtests_test.go

typo

92fcacb

additional output files for hook_errors

7c2e3a9

feature_test

10d581b

feature_test clean up

887b181

fixme fixed and remove john tags

61aea3e

added embiguous steps tests to run.feature

19489aa

typos

dcf0fea

provide a run method that returns the storage so that we can run test…

547f429

…s on it or do whatever the end user wants make honnef scan whole tree

fixed a broken test suite Test_FormatterConcurrencyRun

46d4b2a

Johnlon added 5 commits October 30, 2024 20:22

remove dependency

64026b6

makefile now does same check as github - ie it checks the examples

5b02745

remove @ignore

9a1e216

Change clitest to

dc237f9

go run ./cmd/godog -f progress -c 4 --strict .. because thats what github does

cleanup

535c3c8

Johnlon changed the title ~~DONT MERGE YET - Test renovation - in preparation for fixing the Hook handler bug and adding other features.~~ DONT MERGE YET - REVIEW COMMENTS PLEASE - Test renovation - in preparation for fixing the Hook handler bug and adding other features. Oct 30, 2024

adding missing steps

b35a48b

Johnlon requested a review from vearutop October 31, 2024 01:55

noodnik2 reviewed Oct 31, 2024

View reviewed changes

nhv96 reviewed Oct 31, 2024

View reviewed changes

nhv96 reviewed Nov 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DONT MERGE YET - REVIEW COMMENTS PLEASE - Test renovation - in preparation for fixing the Hook handler bug and adding other features. #659

DONT MERGE YET - REVIEW COMMENTS PLEASE - Test renovation - in preparation for fixing the Hook handler bug and adding other features. #659

Johnlon commented Oct 30, 2024 •

edited

Loading

github-actions bot commented Oct 30, 2024

noodnik2 Oct 31, 2024

noodnik2 Oct 31, 2024

noodnik2 Oct 31, 2024

Johnlon Oct 31, 2024

noodnik2 Oct 31, 2024

Johnlon Oct 31, 2024

noodnik2 left a comment

nhv96 Oct 31, 2024

Johnlon Oct 31, 2024

Johnlon commented Oct 31, 2024

nhv96 Oct 31, 2024 •

edited

Loading

Johnlon Oct 31, 2024

nhv96 Nov 1, 2024

nhv96 Nov 1, 2024

-func NewBase(suite string, out io.WriteCloser) *Base {
-	return &Base{
-		suiteName: suite,
-		indent:    2,
+type baseAdapter struct {
+	out io.Writer
+}
+func (f baseAdapter) Write(p []byte) (n int, err error) {
+	return f.out.Write(p)
+}
+func (f baseAdapter) Close() error {
+	return nil
+}
+// NewBase creates a new base formatter.
+func NewBase(suite string, out io.Writer) *Base {
+	var wc io.WriteCloser
+	w, ok := out.(io.WriteCloser)
+	if ok {
+		wc = w
+	} else {
+		wc = baseAdapter{out: out}
+	}
+	return &Base{
+		suiteName: suite,
+		indent:    2,
+		out:       wc,
+		Lock:      new(sync.Mutex),
+	}
+}
+// Base is a base formatter.
+type Base struct {
+	suiteName string
+	out       io.WriteCloser
+	indent    int
+	Storage *storage.Storage
+	Lock    *sync.Mutex
+}

DONT MERGE YET - REVIEW COMMENTS PLEASE - Test renovation - in preparation for fixing the Hook handler bug and adding other features. #659

Are you sure you want to change the base?

DONT MERGE YET - REVIEW COMMENTS PLEASE - Test renovation - in preparation for fixing the Hook handler bug and adding other features. #659

Conversation

Johnlon commented Oct 30, 2024 • edited Loading

🤔 What's changed?

⚡️ What's your motivation?

🏷️ What kind of change is this?

♻️ Anything particular you want feedback on?

📋 Checklist:

github-actions bot commented Oct 30, 2024

Go API Changes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

noodnik2 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johnlon commented Oct 31, 2024

nhv96 Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Johnlon commented Oct 30, 2024 •

edited

Loading

nhv96 Oct 31, 2024 •

edited

Loading