Skip to content
This repository has been archived by the owner on Apr 19, 2024. It is now read-only.

Added an interface for all the public functions of the survey package #429

Draft
wants to merge 16 commits into
base: master
Choose a base branch
from

Conversation

SirRegion
Copy link
Contributor

This PR adds an interface for survey that can be used instead of calling the functions directly.

It also includes a struct with no contents hat implements the interface.

This lays the groundwork for adding a survey mock as discussed in issue #428.

@mislav
Copy link
Collaborator

mislav commented Jun 8, 2022

Thanks for taking this on! Since this is a work in progress, would you mind marking the PR as draft until it's ready for review?

@SirRegion SirRegion changed the title Added an interface for all the public functions of the survey package Draft: Added an interface for all the public functions of the survey package Jun 10, 2022
@SirRegion
Copy link
Contributor Author

Sure!

@mislav
Copy link
Collaborator

mislav commented Jun 14, 2022

@SirRegion I meant, instead of changing the title of the PR to include the word "draft", actually making the PR as draft in GitHub web UI. I believe that the option to do it is in the right sidebar for the PR.

@SirRegion SirRegion marked this pull request as draft June 20, 2022 07:25
@SirRegion
Copy link
Contributor Author

Oh, thank you. I'm way more familiar with GitLab than Github, where adding "draft:" to an PR automatically converts it. I fixed it now :)

@SirRegion SirRegion changed the title Draft: Added an interface for all the public functions of the survey package Added an interface for all the public functions of the survey package Jun 20, 2022
@SirRegion SirRegion marked this pull request as ready for review June 20, 2022 11:57
@SirRegion
Copy link
Contributor Author

I added an implementation for the mock (including tests) and some documentation in the readme on how to use it.

You can now start reviewing and merging if you think it's ready :)

Copy link
Collaborator

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

This looks promising! Thank you for working on it.

Over in the GitHub CLI codebase we have experience with mocking Survey using our own approach. One thing we learned is that our tests are more resilient over a longer period of time if we mock Survey in a way that the mocks are still valid even if the underlying implementation switches from Ask to AskOne (and vice versa). Also, we have ability to mock multiple sequential calls of Ask/AskOne. We do this by matching result stubs not to Ask/AskOne calls, but to prompt messages. What do you think of this approach, and would you be open if I helped port it over to what you are proposing in your PR?

surveyMock.go Outdated Show resolved Hide resolved
//create mock
mock := survey.SurveyMock{}
//set the response the "user" should select
mock.setResponse(true)
Copy link
Collaborator

Choose a reason for hiding this comment

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

If AskForPie is going to call survey.Ask() multiple times, how would we mock all those using SetResponse?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current implementation does not allow to set multiple responses and would always use the last one that is set.
However, I feel like this could be acceptable behaviour, because it forces the developers to separate different ask calls into multiple functions that only ever do one thing (as they should 😉)

Alternatively, setResponse() could return the struct and store the responses in a slice. This would make this call joinable and allow multiple answers.

Copy link
Collaborator

@mislav mislav Jun 21, 2022

Choose a reason for hiding this comment

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

I feel strongly that the mock solution should be flexible enough to mock all responses, no matter how many times Ask or AskOne has been called. I may be able to contribute this improvement to your mock implementation.

Example use case:

surveyer := survey.SurveyMock{}
surveyer.SetReponse(map[string]interface{}{
  "name": "foo",
  "message": "hello",
})

DoSomething(surveyer)

This test setup will work as long as DoSomething is calling Ask with two questions. However, let's say that the implementation of DoSomething needs to change because the prompt of question 2 needs to dynamically adjust to the response from question 1. The way to do that with survey is to switch from Ask to 2x AskOne. After the implementation changes in this way, the current test from the example will break. However, if the mock solution does not support stubbing several AskOnes, then the user will be forced to break up DoSomething into separate functions just to appease the SurveyMock implementation. I think this would cause frustration to the user.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is a usecase i did not think of before.

How would you feel about something like this (inspired by jest):

mock := survey.Surveyor{}
mock.SetResponseOnce("first").SetResponseOnce("second").SetRespose("all others")

DoSomething(surveyor)

Now the mock would respond to the first question inside DoSomething with "first", two the second with "second" and to all other questions with "all others".

Now if DoSomething is using Ask, it would just fill the question struct with the answers "first", "second", "all others", "all others" if the prompt had four questions.

If DoSomething is using AskOne, it would respond to the first AskOne with "first", and to the second with "second", etc...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I like your proposal; it looks more flexible already! One thing that I still think would be great if SurveyMock could do is verify that the prompt was shown to the user with the right text, since this text is the most important part of how the user interacts with Survey. We do it like this, which feels similar to what you've proposed, but allows for a greater level of precision.

Copy link
Owner

Choose a reason for hiding this comment

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

I think if we're going down this path we probably also want to provide methods on the mock for setting a specific field with a value (something like .setFieldResponse("name", "SirRegion"). I'm not sure there isn't a lot of value in coupling a test to the order in which the questions are asked but I think if someone wants to structure there test that way, we should support them

Copy link
Collaborator

Choose a reason for hiding this comment

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

@SirRegion Yes, let's undo the overall README changes for now (to keep things as they were before) and have there just be a small mention & example of the mock functionality.

@AlecAivazis How would the SetFieldResponse() functionality allow for asserting the order in which questions were asked?

Copy link
Owner

Choose a reason for hiding this comment

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

I mean to have both methods so a user could decide if they want to test the order of responses or just if a response was given for a field.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@AlecAivazis Agreed! It's probably better to be strict with verification out-of-the-box (i.e. to verify the order of prompts by default) but give the developer a chance to opt out if needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mislav I reverted the commit that changed the readme for now and updated the Testing part to include a warning about the unstable mock API.

prompt := &survey.Confirm{
Message: "Do you like pie?",
}
survey.AskOne(prompt, &response)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Now the big question is whether we should amend all other examples in the Readme to call Ask and AskOne on the Survey struct instead of on the package level? That would make everyone's code ultimately testable by default. Otherwise, the Readme will offer users usage examples of survey on the package level, but later the testing section will effectively tell them they need to rewrite their original code if they hope to mock Ask/AskOne calls. My hope for Survey is that it's testable by default without the need of changing the original implementation. @AlecAivazis Thoughts?

Copy link
Contributor Author

@SirRegion SirRegion Jun 21, 2022

Choose a reason for hiding this comment

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

I guess updating the readme should not be to much work, as it's just one or two lines of code changed in each example.

I left it the same for this pull request, because I wasn't sure how much of a "breaking" change you would accept for the docs.

If we decide to update the docs, I would take on the work!

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 updated the examples in the readme, but I did not touch the text for now.

Some parts ("Running the Prompts", "Testing") likely still need updating.

Copy link
Owner

Choose a reason for hiding this comment

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

Hm, good question. I think keeping the examples as short as possible is probably the best. I'd like to avoid the surveyor := survey.Surveyor{} at the start of every example so either we can reference an undefined surveyor in the examples or we can just use the package-level functions. Either way, we definitely want to add a section about these two different approaches in the "Running the Prompts" section so it shouldn't be too confusing if we do decide to update the tests to reference surveyor.Ask without explicitly showing its declaration.

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 think explicitly using surveyor.Ask in the examples is the way to go, because it makes the code testable by default (like @mislav noted).

Referencing an undefined surveyor is probably not even a bad move, because it might move users directly to passing it to functions instead of creation a new instance everytime they want to ask a question.

I would recommend updating the first example to include a function where the surveyor is passed as an argument to show how it is done "correctly".

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@SirRegion
Copy link
Contributor Author

This looks promising! Thank you for working on it.

Over in the GitHub CLI codebase we have experience with mocking Survey using our own approach. One thing we learned is that our tests are more resilient over a longer period of time if we mock Survey in a way that the mocks are still valid even if the underlying implementation switches from Ask to AskOne (and vice versa). Also, we have ability to mock multiple sequential calls of Ask/AskOne. We do this by matching result stubs not to Ask/AskOne calls, but to prompt messages. What do you think of this approach, and would you be open if I helped port it over to what you are proposing in your PR?

Thanks for your suggestions. I basically just build an MVP that is enough for my purposes, but I would be happy to see you implement a more sturdy variant on top of the work I already did.
I think it would be great to have both the ability to define the response for Ask/AskOne directly and also have the more complex way of returning an answer based on the prompt given.

@AlecAivazis AlecAivazis marked this pull request as draft September 22, 2022 21:54
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants