Repository navigation
Add tests for Interactive Forms + minor validation tweaks - #6098
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
89dcf96 to
a84fb89
Compare
This adds some tests for `form.ts`, plus some minor changes to the validation for consistency, including: - consider empty strings as "no answer" (eg. fail validation if required) - treat empty inputs as "null" answer for optional fields - validate individual parts of a list input - use `null` for skipped answers of optional fields POTENTIALLY BREAKING: For consistency, we always use `null` for skipped optional answers now. Previously an optional `list` field would get an empty array if skipped.
a84fb89 to
b3b7a47
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## interactive-forms #6098 +/- ##
=====================================================
+ Coverage 70.01% 71.10% +1.08%
=====================================================
Files 173 173
Lines 13273 13295 +22
Branches 2668 2677 +9
=====================================================
+ Hits 9293 9453 +160
+ Misses 3484 3322 -162
- Partials 496 520 +24 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
DanTup
left a comment
DanTup
left a comment
There was a problem hiding this comment.
@h9jiang @bwilkerson this PR is mostly adding some tests (which I've tried to keep standalone in case we want to lift this with form.ts to a central repo in future), however while adding them I noticed some slight inconsistencies with validation (empty strings, and lists) so there are also some minor changes in form.ts.
| return []; | ||
| // Treat empty as no answer. We shouldn't get here if required because | ||
| // of validation. | ||
| return null; |
There was a problem hiding this comment.
@h9jiang I made this change to keep things consistent (submitting an empty input for an optional field always uses null for the answer), however if you've already shipped any server code with optional lists that expect non-null, this might be breaking. If so, we could revert this.
|
@h9jiang I'd also like your feedback on these changes to |
h9jiang
left a comment
h9jiang
left a comment
There was a problem hiding this comment.
I think the CL make perfect sense to me.
Right now, the vscode-go test against the gopls directly. However the approach you are taking (testing against the dummy language server) is better.
Thanks for coming up with the robust test!
|
Great, thanks for the review! 🙂 |
This adds some tests for
form.ts, plus some minor changes to the validation for consistency, including: