This repository has been archived by the owner on Apr 19, 2024. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Added an interface for all the public functions of the survey package #429
base: master
Are you sure you want to change the base?
Added an interface for all the public functions of the survey package #429
Changes from all commits
cc375fe
dc306a6
5e8fe73
155eb18
ed80995
96b15b2
80f6737
a462a5c
b9c3cb5
5b78430
69f3ff0
5ba9322
79a6d75
fe82ce8
f62075a
d70c6af
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
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
andAskOne
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?There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 undefinedsurveyor
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 referencesurveyor.Ask
without explicitly showing its declaration.There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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 usingSetResponse
?There was a problem hiding this comment.
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.There was a problem hiding this comment.
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:
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.
There was a problem hiding this comment.
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):
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 usingAsk
, 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 usingAskOne
, it would respond to the firstAskOne
with "first", and to the second with "second", etc...There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 themThere was a problem hiding this comment.
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?There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.