Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

cweave
Copy link
Contributor

@cweave cweave commented Oct 13, 2022

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!

  • There's a clear use-case for this code change, explained below
  • Commit message has a short title & references relevant issues
  • The build will pass (run yarn test:all and yarn 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

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
Copy link

codecov bot commented Oct 13, 2022

Codecov Report

Base: 52.32% // Head: 52.33% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (d631423) compared to base (74e1d52).
Patch coverage: 100.00% of modified lines in pull request are covered.

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     
Impacted Files Coverage Δ
ghost/core/core/frontend/helpers/t.js 82.50% <100.00%> (+20.33%) ⬆️
ghost/admin/app/helpers/gh-price-amount.js 44.44% <0.00%> (-22.23%) ⬇️

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.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Member

@ErisDS ErisDS left a 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?

@github-actions
Copy link
Contributor

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 🙏

@ErisDS
Copy link
Member

ErisDS commented Oct 31, 2022

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 hacktoberfest-accepted anyway as I was the one who blocked this and I know you worked hard on it ❤️

@cweave
Copy link
Contributor Author

cweave commented Oct 31, 2022

@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 ☺️

@ErisDS ErisDS self-assigned this Nov 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants