Skip to content

New prompt testing utility#7116

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

New prompt testing utility#7116
vilmibm merged 3 commits intotrunkfrom
even-more-prompts

Conversation

@vilmibm
Copy link
Contributor

@vilmibm vilmibm commented Mar 9, 2023

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.

Copy link
Contributor

@mislav mislav left a comment

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 👍

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.

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

@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
@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
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
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.

ConfirmDeletionStubs []ConfirmDeletionStub
}

// TODO thread safety
Copy link
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
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.

4 participants