Skip to content

Improve Survey stubber for tests#5032

Merged
mislav merged 12 commits intotrunkfrom
ask-stubber
Jan 17, 2022
Merged

Improve Survey stubber for tests#5032
mislav merged 12 commits intotrunkfrom
ask-stubber

Conversation

@mislav
Copy link
Contributor

@mislav mislav commented Jan 13, 2022

Both SurveyAsk and SurveyAskOne methods now share the same sets of stubs, making it possible to change which of these methods is used in the implementation without breaking tests.

A new method AskStubber.StubPrompt("<prompt>") is added as test helper to supersede old Stub and StubOne methods. The new helper matches on prompt messages rather than on field names, enabling tests to be written based on what the user would see rather than coupling to implementation details.

The new stubber also allows verifying whether a Select or MultiSelect was rendered with the expected set of options. Furthermore, if a stubbed value is not present among those options, the stubber will panic instead of continuing normally.

Stubbed Selects with an int instead of a string target receiver are now transparently handled. The values for Select stubs are always strings in tests, but the stubber will write an int answer if the receiver expects one as a selected index instead of a selected string value.

Lastly, this set of changes improves test resiliency since the stubs are now matched based on prompt message (or field name for legacy stubs created with Stub) instead of sequentially, enabling the implementation to reorder the prompts without breaking existing tests.

Because the new stubbing API is no longer concerned with Survey internals, this allows us to potentially transition off of Survey in the future but keep the test stubs.

Some before vs. after:

// BEFORE
	as.StubOne(0) // template
	as.Stub([]*prompt.QuestionStub{
		{
			Name:    "Body",
			Default: true,
		},
	}) // body
	as.Stub([]*prompt.QuestionStub{
		{
			Name:  "confirmation",
			Value: 0,
		},
	}) // confirm

// AFTER
	as.StubPrompt("Choose a template").
		AssertOptions([]string{"Bug fix", "Open a blank pull request"}).
		AnswerWith("Bug fix")
	as.StubPrompt("Body").AnswerDefault()
	as.StubPrompt("What's next?").
		AssertOptions([]string{"Submit", "Continue in browser", "Add metadata", "Cancel"}).
		AnswerDefault()

Both SurveyAsk and SurveyAskOne methods now share the same sets of
stubs, making it possible to change which of these methods is used in
the implementation without breaking tests.

A new method `AskStubber.StubPrompt("<prompt>")` is added as test helper
to supersede old Stub and StubOne methods. The new helper matches on
prompt messages rather than on field names, enabling tests to be written
based on what the user would see rather than coupling to implementation
details.

The new stubber also allows verifying whether a Select or MultiSelect
was rendered with the expected set of options. Furthermore, if a stubbed
value is not present among those options, the stubber will panic instead
of continuing normally.

Stubbed Selects with an int instead of a string target receiver are now
transparently handled. The values for Select stubs are always strings in
tests, but the stubber will write an int answer if the receiver expects
one as a selected index instead of a selected string value.

Lastly, this set of changes improves test resiliency since the stubs are
now matched based on prompt message (or field name for legacy stubs
created with Stub) instead of sequentially, enabling the implementation
to reorder the prompts without breaking existing tests.
@mislav mislav requested a review from a team as a code owner January 13, 2022 10:09
@mislav mislav requested review from samcoe and removed request for a team January 13, 2022 10:09
Copy link
Contributor

@samcoe samcoe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is sooo much nicer than before! I love it. Left just a couple questions. Also looks like the linter is not passing, was the plan to fix all those before merging or overtime making those changes?

panic(fmt.Sprintf("more asks than stubs. most recent call: %#v", qs))
var stub *QuestionStub
for _, s := range as.stubs {
if !s.matched && (s.message == "" && strings.EqualFold(s.Name, fieldName) || s.message == message) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supporting regex matching on the message could be a nice improvement for the future, not necessary to add now.

@mislav
Copy link
Contributor Author

mislav commented Jan 14, 2022

@samcoe I've ported some more legacy stubs, but there are many more remaining, so I've allow-listed remaining ones with //nolint directives.

@mislav mislav requested a review from samcoe January 14, 2022 19:05
@mislav mislav merged commit a3f0940 into trunk Jan 17, 2022
@mislav mislav deleted the ask-stubber branch January 17, 2022 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants