New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Auto-focus and select file name for create new spec modal #22284
Merged
Merged
Changes from all commits
Commits
Show all changes
11 commits
Select commit
Hold shift + click to select a range
4fd9ed4
feat: Auto-focus and select file name for create new spec modal
astone123 2c836a2
feat: Add comment for regex
astone123 b3eab6d
feat: Remove comma; add new line
astone123 7b99f29
Merge branch 'develop' into 21865-create-spec-autofocus
astone123 da055b9
feat: Remove unnecessary function
astone123 cfb75d4
feat: Remove unnecessary logic
astone123 710dfbc
Merge branch 'develop' into 21865-create-spec-autofocus
ZachJW34 5de7ed0
feat: Reference prop value rather than input ref value
astone123 50851bd
feat: Add component test for unhappy path
astone123 f7fd80b
Merge branch 'develop' into 21865-create-spec-autofocus
astone123 ab1e40d
Merge branch 'develop' into 21865-create-spec-autofocus
astone123 File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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.
As a nit/discussion item but not a hard blocker, I want to get your opinion on how we should be testing this kind of code, specifically the "business logic" part, which is parsing the spec.
Eg, I wonder if a pure function in a separate module (say 'specModal.ts', or whatever) like this would be better:
It would be easy to test in isolation (no need for the UI framework to come into play), and against lots of different strings - right now, we only test it against one string, which imo isn't ideal for regexp - I'd like to at least see a case where the string doesn't match, and see what happens.
We should test for the unhappy path, too, not just the happy path, and this would be an easy way to express many of those. I'm not sure the ideal of re-mounting a component many times for lots of regexp test is ideal.
Finally, you'd the import this into the component, and your
onMount
would beMore a point of discussion around best practices and testing philosophy in general, thoughts? It's a bit more overhead, but I like the separation of concerns.
Again this change isn't blocking the PR - it's more to solicit discussion and help guide us as we define the best practices for component tests, and so we can educate the rest of the company, and our users.
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.
Yeah this is a good discussion to have. I like the idea of having at least one component test that tests the "happy path" just so that we are sure that in most cases the component is behaving as expected. Having at least one test that covers everything together is crucial imo. But I also have this same thought
One thing I'd question about that though: is the regex the only thing we'd be testing? I'd argue that it's not. What's important is the experience for the user for different situations (i.e. when the default file path that we provide doesn't match the regex) which I think we can get better from a CT.
So at this point my thought is to add more component test cases that provide different default strings and see how the component behaves. That's probably what I'll do to finish out this PR, but I'm not sure if I love that approach because it doesn't seem very efficient.
I also like the idea of separating the logic out into its own function like
getSelectionRange
for the sake of cleaning things up a bit if nothing else. As far as using CT vs a normal unit test for something like this, I think I'm torn. Would like to hear what the rest of the team thinksThere 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.
Good points! No right or wrong here.
For now, happy with the unhappy case you've added: 50851bd
✅