add new option for created features with parsing from byte slices

[akaswenwilk]

Hello! In my work we are trying to build in a pre-processor for our feature files so we can have some dynamic feature generation when running our tests. I made this for us to use but though maybe it could be interesting for more people than just us. I've added another option to define custom features to be used in the suite, and then added an interface to pass in a map[string][]byte representing the path/read contents from the file (in our case, the contents after doing preprocessing). I think this implementation is maybe not fully working since it seems to be missing the line filtering, but other than that I think I've got all the other inner workings there. Just wanted to submit it if it could be interesting for anyone!

Leave me a comment if you think this is a good idea and then I can try adding some testing/additional documentation

🤔 What's changed?

⚡️ What's your motivation?

🏷️ What kind of change is this?

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

♻️ 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.

[vearutop]

This is an interesting feature and I think it can be very helpful to improve portability/embedability of tests. Let me check it in more detail to see if the API of this can be improved to be more accessible.

[mattwynne]

Thanks for sharing! Can you add some tests for this @akaswenwilk?

[akaswenwilk]

Thanks for sharing! Can you add some tests for this @akaswenwilk?

Sure thing!

[akaswenwilk]

hi @mattwynne , so sorry for the delay to get back to this. I've added some tests for the ParseFromBytes function in the parser, but now I'm trying to address the interface a bit. Currently I've added a Features option where someone can manually pass in a slice of features, but maybe actually it makes more sense to simple add a different option instead (like FeatureContents) which would be a map[string][]byte of all the features to be run. Otherwise the user has to generate a slice of Feature models using the RetrieveFeaturesFromBytes method. Any thoughts on which might be nicer? At which point I can try adding a test in run_test.go and maybe we can finalize this? Thanks for your help!

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (main@b2672bb). Click here to learn what that means.
The diff coverage is 82.05%.

@@           Coverage Diff           @@
##             main     #476   +/-   ##
=======================================
  Coverage        ?   81.69%           
=======================================
  Files           ?       27           
  Lines           ?     2267           
  Branches        ?        0           
=======================================
  Hits            ?     1852           
  Misses          ?      316           
  Partials        ?       99           

Impacted Files	Coverage Δ
test_context.go	77.19% <ø> (ø)
run.go	75.77% <76.92%> (ø)
internal/parser/parser.go	64.42% <84.61%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b2672bb...afa6081. Read the comment docs.

[akaswenwilk]

@vearutop, sorry for the delay, but how about this for improved API? This way the user can just pass in a map[string][]bytes directly as an option and not have to deal with trying to first parse their bytes into a feature to pass in as an option. Could be a bit simpler?

[vearutop]

Let's make this a slice of structs to improve clarity of data items.
Something like:

	// Features allow passing in additional features manually.
	Features []Feature
}

// Feature contains path and contents of a gherkin file.
type Feature struct {
	Path    string
	Content []byte
}

[vearutop]

Let's change logic here so that both Paths and bytes can work together. We can check for non-empty opt.Paths and append runner.features.

[akaswenwilk]

hmm, I'm not so sure how simple this would be. From what I can see, if someone wants to run through FeatureContents only, and they leave the Paths option blank, the current behavior will force them to use a features directory by default. Either they'll have their features already in a features directory, or they won't, in which case there could be a failure due to non-existent directory, no? I think it's maybe just simpler to say, either you can use feature contents, or features, but not both. If someone is already parsing files and passing them in, I can't imagine it's much more challenging to also include from all features as opposed to from only a few. what do you think?

[vearutop]

Hm, this is interesting point.

If someone is already parsing files and passing them in, I can't imagine it's much more challenging to also include from all features as opposed to from only a few.

Another case when this feature could be useful would be sourcing gherkin files from a remote storage (e.g. via HTTP) or generating/templating them by code. In such situation implementing additional file preloader might be a burden.

If we disable that default in case of feature contents presence it may be perceived as unexpected behavior.

	if len(opt.Paths) == 0 && len(opt.Features) == 0 {
		inf, err := os.Stat("features")
		if err == nil && inf.IsDir() {
			opt.Paths = []string{"features"}
		}
	}

If we ignore Paths (make feature contents and paths mutually exclusive) this can also be perceived as unexpected behavior (that needs to be documented or maybe even have a check and explicit failure to not mislead user).
And implementing preloader can be a minor annoyance for a user in case they want both types of features.

From these options I'm leaning to disabling the default as it seems less disruptive overall.

[akaswenwilk]

Sounds good!

[vearutop]

@akaswenwilk overall looks good to me (apart from 2 minor suggestions that I left). 👍

[wichert]

I'm wondering if it makes sense to provide an ability to pass in an fs.FS instance, along with a wrapper to expose your map as something that implements FS.

[vearutop]

I think fs.FS does not have enough info (you need to know names in advance).

// An FS provides access to a hierarchical file system.
//
// The FS interface is the minimum implementation required of the file system.
// A file system may implement additional interfaces,
// such as ReadFileFS, to provide additional or optimized functionality.
type FS interface {
	// Open opens the named file.
	//
	// When Open returns an error, it should be of type *PathError
	// with the Op field set to "open", the Path field set to name,
	// and the Err field describing the problem.
	//
	// Open should reject attempts to open names that do not satisfy
	// ValidPath(name), returning a *PathError with Err set to
	// ErrInvalid or ErrNotExist.
	Open(name string) (File, error)
}

We could use fs.ReadDirFS

// ReadDirFS is the interface implemented by a file system
// that provides an optimized implementation of ReadDir.
type ReadDirFS interface {
	FS

	// ReadDir reads the named directory
	// and returns a list of directory entries sorted by filename.
	ReadDir(name string) ([]DirEntry, error)
}

to be able to traverse entries, but I'm not sure it would be more convenient to use instead of a list of named bytes.

[aslakhellesoy]

Hi @akaswenwilk,

Thanks for your making your first contribution to Cucumber, and welcome to the Cucumber committers team! You can now push directly to this repo and all other repos under the cucumber organization! 🍾

In return for this generous offer we hope you will:

• ✅ Continue to use branches and pull requests. When someone on the core team approves a pull request (yours or someone else's), you're welcome to merge it yourself.
• 💚 Commit to setting a good example by following and upholding our code of conduct in your interactions with other collaborators and users.
• 💬 Join the community Slack channel to meet the rest of the team and make yourself at home.
• ℹ️ Don't feel obliged to help, just do what you can if you have the time and the energy.
• 🙋 Ask if you need anything. We're looking for feedback about how to make the project more welcoming, so please tell us!

On behalf of the Cucumber core team,
Aslak Hellesøy
Creator of Cucumber