Skip to content

New prompt testing utility - #7116

Merged
vilmibm merged 3 commits into
trunkfrom
even-more-prompts
Mar 10, 2023
Merged

vilmibm merged 3 commits into
trunkfrom
even-more-prompts

Conversation

@vilmibm

@vilmibm vilmibm commented Mar 9, 2023 •

Copy link
Copy Markdown
Contributor

This PR implements a more robust way to make assertions about prompts; awkwardy called MockPrompter. It wraps the auto-generated PrompterMock and adds functions for registering expectations and verifying them.

to use it in a test:

pm := prompter.NewMockPrompter(t)
defer pm.Verify()

pm.RegisterSelect("animal type?", []string{"cat", "dog", "moose"}, func(_, _ string, opts []string) (int, error) {
  return prompter.IndexFor(opts, "cat")
}
pm.RegisterInput("cat name?", func(_, d string) (string, error) {
  return d, nil
}
pm.RegisterInput("cat color?", func(_, _ string) (string, error) {
  return "tortoiseshell", nil
}
pm.RegisterConfirm("hug the cat?", func(_ string, _ bool) (bool, error) {
  return true, nil
}

The MockPrompter ensures that:

  • unexpected prompts return an intelligible error
  • expected prompts are all seen and show up in the order they were registered

and avoids any interface{} shenanigans.

This PR includes updates to two test files to illustrate the new approach.

@mislav mislav left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice work!

I'm considering accepting a prompt in the Register* functions as the first argument so the caller doesn't have to do the NoSuchPromptErr themselves.

I'd like to see that 👍

@samcoe samcoe left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I like this approach. Agreed that adding the prompt as the first argument to Register functions would be a nice improvement.

Comment thread internal/prompter/test.go Outdated
@vilmibm
vilmibm force-pushed the even-more-prompts branch 2 times, most recently from 034b5d0 to 21fbe57 Compare March 9, 2023 23:50
@vilmibm
vilmibm force-pushed the even-more-prompts branch from 0173f97 to 10dd74b Compare March 10, 2023 00:40
@vilmibm
vilmibm marked this pull request as ready for review March 10, 2023 00:43
@vilmibm
vilmibm requested a review from a team as a code owner March 10, 2023 00:43
@vilmibm
vilmibm requested review from mislav and removed request for a team March 10, 2023 00:43
@vilmibm
vilmibm requested a review from samcoe March 10, 2023 00:44
@vilmibm vilmibm mentioned this pull request Mar 10, 2023
23 tasks done
@vilmibm vilmibm changed the title proof of concept for Prompter testing interface New prompt testing utility Mar 10, 2023
type Prompter interface {
Select(string, string, []string) (int, error)
MultiSelect(string, string, []string) (int, error)
MultiSelect(string, string, []string) ([]string, error)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For symmetry with Select, would we return []int here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

i'm torn about it. no code is actually relying on the new MultiSelect so i haven't played with it (hence the bug) to see how cumbersome a list of ints is to work with. i'll make a call when i'm actually porting a MultiSelect which will be soon.

Comment thread internal/prompter/test.go
ConfirmDeletionStubs []ConfirmDeletionStub
}

// TODO thread safety

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

maybe it's okay if these methods aren't threadsafe, prompts in the UI cannot happen in parallel anyway?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ah good point

@vilmibm
vilmibm merged commit 661d962 into trunk Mar 10, 2023
@vilmibm
vilmibm deleted the even-more-prompts branch March 10, 2023 18:43
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.

5 participants