-
-
Notifications
You must be signed in to change notification settings - Fork 10.2k
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
🎨 add unit tests for HB translate helper #15607
base: main
Are you sure you want to change the base?
Conversation
Added unit tests to the MR TryGhost#15529 / issue TryGhost#15500. Added a regex in the `t()` to explicitly check for smart apostrophes in the event that they are not stripped out/come through as undefined.
Codecov ReportBase: 52.32% // Head: 52.33% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## main #15607 +/- ##
=======================================
Coverage 52.32% 52.33%
=======================================
Files 1446 1446
Lines 93513 93516 +3
Branches 10439 10440 +1
=======================================
+ Hits 48932 48940 +8
+ Misses 43353 43347 -6
- Partials 1228 1229 +1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
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.
Hey @cweave, so sorry for taking an age to get back to you on this. I looked at it when you did it and wanted to sort out an issue with our existing tests to try to help with this PR and it's taken me an age as we've been shipping some pretty big features 😬
Finally I got it done today. You can see my changes which add a test utility and then extend it for testing errors.
I think in the case of this PR, using the function call method isn't accurately testing how this works with smart quotes, because the quotes are inside the string, rather than wrapping it.
However, I think if you use the new shouldCompileToError
util, it may be possible to write a test that more closely matches how this works in reality.
So hopefully it'll be able to change do something like shouldCompileToErrr('{{t “This causes an error”}}', {}, {name: 'IncorrectUsageError'})
.
I hope this makes some sense?
Note from our bot: Some changes have been requested on this pull request. Updating your code is great, but won't notify us, so please leave a comment so that we (and our bot) can see when you've made the changes. Thank you 🙏 |
I know it's been a long time since you opened this and you might not have time to come back to it now (although I hope you will). I've added |
@ErisDS I appreciate tagging it! I will come back and do the updates! I haven't had a chance to compare your comments with the code yet, but I think it make sense. When I was writing the tests, I think I was experiencing a little bit of bugginess and felt that the test was a little flakey, but it was consistently producing the same results by time I put the PR in. So, I'll take a look at all that and get an update in |
Added unit tests to extend on my MR #15529 / issue #15500. Added a regex in
t()
to explicitly check for smart apostrophes in the event that they are not stripped out/come through as undefined.Participating inHacktoberfest :)
Got some code for us? Awesome 🎊!
Please include a description of your change & check your PR against this list, thanks!
yarn test:all
andyarn lint
)We appreciate your contribution!
Also, if you'd be interested in writing code like this for us more regularly, we're hiring:
https://careers.ghost.org