Convention Decision: to i18n or not to i18n in spec assertions

Tech
1

The objective here is to close the discussion started here.

It ended up here, looking like this:

### Never use I18n.t in assertions

When asserting text that is translatable, always assert against the translated string, not the call to I18n.t.

Good  `expect(json_response['errors']).to eq "Hm, something went wrong. No order cycle data found."`

Bad  `expect(json_response['errors']).to eq I18n.t('admin.order_cycles.bulk_update.no_data')`

But some people disagree.
I dont have an opinion except that I think we need a decision, one way or the other. Having the code mixed up with the two approach is very poor practice…

@luisramos0
I thought we should use t(translation_key) in specs and not the text itself

@maikel
Upside: We can change text without changing the spec.
Downsides:

  • The spec doesn’t detect some translation mistakes:
    • Duplicate keys often lead to the selection of the wrong key.
    • A typo in the key (used in the view) may not be detected.
    • Mistakes in string interpolation can’t be tested.
  • The spec is harder to read and you don’t know immediately what is displayed.

@luisramos0
I really like to see the I18n.t on the specs, looks very clean, but your arguments against it are very good!

@Matt-Yorkley
I tend to prefer using I18n.t so things can be reworded without changing the specs. Spelling mistakes in the view translation key would still be caught, no? Maybe the usefulness of either option depends on the situation, like with interpolated keys it might be better to check the full output.
Not being able to read the message whilst scanning the specs is a fair one, but my IDE displays the translation automatically when viewing keys in specs, so I’m pretty ambivalent on that one…

@kristinalim
I also vote not to use I18n.t in the tests. :slightly_smiling_face:

Spelling mistakes in the view translation key would still be caught, no?

Hmm… @Matt-Yorkley Yes, you’re right, it would catch a typo in the base key, although I think not mistakes in the scope.

Nice list of downsides, @mkllnk. An addition to that, if you have missing keys:

expect(“translation missing: en.missing_key”).to eq(“translation missing: en.missing_key”)

@luisramos0
I’d consider it closed. No one disagrees with this convention, right? link_to_convention

@sauloperez
I actually disagree @luisramos0 . It allows running the specs in any language and provides an extra abstraction that lets you change the copy without having to touch the test. See Better Tests Through Internationalization.

2

@Matt-Yorkley, your IDE tip sounds handy. I also like the tips in the thoughtbot article that @sauloperez linked, and there is also this issue I created which lists some tasks.

I thought it over again, and :thinking: I’m happy to change my vote if we do these (consolidated from above):

  1. (Development (Personal)) IDE plugin - @Matt-Yorkley, what plugin do you use? And the thoughtbot article mentions vim-i18n for vim users. I had no idea these tools exist! Really nice.
  2. (Ruby) Raise error in test and, optionally, development environments when a translation is missing - Sample code in thoughbot article
  3. (Browser) Highlight span.translation_missing - Not necessary if we also raise an error in the development environment for Task 2
  4. (Translation management) Sort translations in the YAML file - The thoughbot article mentions i18n_yaml_sorter.
  5. (JS) Alert or console error logging in test and development environments if a string translated in the JS is missing
3

Thank you @luisramos0 for bringing it all together and thank you @kristinalim for all the suggestions. Let’s look at the issues one by one.

Missing translations

I tried the exception raising for tests in https://github.com/openfoodfoundation/openfoodnetwork/commit/fd56615fc6edc880ce5a11656b3fc31bc98e52df and that works nicely. :heavy_check_mark:

Duplicate keys

yamllint can detect duplicate keys. We could include that in our tests:

pip install yamllint
yamllint config/locales/en.yml

There are a few errors in the current file. It shows how much we need this tool.

A typo in the key

I meant a typo that results in a key that actually exists. For example instead of example.product.error somebody types example.products.error and then we display the wrong message. That actually happened. Any idea how to tackle this?

Mistakes in the string interpolation

For example t(:message, user: user) is supposed to display “Welcome John” and is defined as Welcome %{user.name}. Now there are two common mistakes.

  1. The dev writes Welcome %{user} and we end up with “Welcome <Spree::User id…>”.
  2. The dev writes Welcome #{user.name} resulting in “Welcome #{user.name}”.

I’ve seen both of these and they were not always detected in code review. :frowning: Any ideas how to prevent these mistakes?

Not seeing the translation immediately

Okay, it seems like RubyMine displays it (Matt’s choice). And It’s not the most important thing to me. I’m okay to compromise this for having the text only in one place.

4

Hmm… If we require all code reviewers to have an IDE plugin for dealing with translations, maybe we can treat this like any other wrong copy-paste of string which should/would not get approved during code review.

No ideas for the string interpolation issue… :thinking: :slightly_frowning_face:

5

Agree overall. Up for adopting i18n.t in specs.

As for the string interpolation issue, is as big as all the other problems? although it could happen the first time the dev works with Rails’ I18n and while learning, this can be catched not only by code reviewing but also by testing. Even if with all this we don’t catch the bug, having users report it is not that big deal. It’s not stopping them from using the platform.

So to take action items out of this:

  1. (Development (Personal)) IDE plugin - @Matt-Yorkley, what plugin do you use? And the thoughtbot article mentions vim-i18n 1 for vim users. I had no idea these tools exist! Really nice.

Shall we ask devs to install these plugins somewhere in the docs? Btw, @kristinalim I gave it a try with neovim and we might have a too old Ruby version in OFN…

  1. (Ruby) Raise error in test and, optionally, development environments when a translation is missing - Sample code in thoughbot article

Let’s create an issue and tackle it.

  1. (Browser) Highlight span.translation_missing - Not necessary if we also raise an error in the development environment for Task 2

Agree. Not needed.

  1. (Translation management) Sort translations in the YAML file - The thoughbot article mentions i18n_yaml_sorter.

Let’s also create an issue to integrate it in our app.

  1. (JS) Alert or console error logging in test and development environments if a string translated in the JS is missing

An another issue for this one.

Does everyone agree? Who is up for leading this and create these issues? You @kristinalim seem to have gone through it in-depth, or perhaps you @luisramos0 ?

6

I don’t have a really strong preference either way, but I think we should pick one or the other.

I’m still not sure about the hypothetical problems mentioned above, I think I agree with Pau that these problems will be caught in development / code review / testing as part of the usual processes.

7

@maikel What do you think as about trusting that those dev errors would be caught during code review and manual testing?

@luisramos0 You’re the one who initiated this discussion - do you want to take care of the follow-up actions? If not particularly, I can do it.

8

Yes, I’m okay with that. We have pretty good testing. I just mentioned it, because it passed testing in the past and it will happen again. That’s a trade-off for prettier code that’s easier to maintain.

Sorry, I just thought of another issue weighing against I18n in specs. If we move translation keys, we have to change the specs as well. That happens easily when moving parts of a growing view into partials. So the question is: What happens more often? Changing the English translation or changing the translation key? If we don’t know the answer, I don’t see any upside of using I18n. :man_shrugging:

Anyway, I see a majority for using I18n here and if people are happier with the code that way, that’s okay with me. It’s more important to be consistent than which way we go.

9

Good stuff! I’ve inverted the convention to always user i18n.t here.

Ive installed rails i18n helper on my sublime, it’s too slow and doesnt work with symbols like t :label_login only strings.

And there are other actions, right?

  1. (Ruby) Raise error in test and, optionally, development environments when a translation is missing - Sample code in thoughbot article
  2. (Translation management) Sort translations in the YAML file - The thoughbot article mentions i18n_yaml_sorter.
  3. (JS) Alert or console error logging in test and development environments if a string translated in the JS is missing

1 and 3 - any takers?

2 - I dont get the point. Entries should be organised logically, when you add something you have to think, what’s the lookup hierarchy you should use and where it should belong, just like what you do with code. Relying on a task to sort alphabetically sounds lazy to me.

10

I think the numbers don’t match. Did you mean 1 and 3 and then 2?

To be honest, I’d like someone other than me to take this. My tasks are piling up already… although that might be the case for everyone :sweat_smile:

11

thanks Pau, I have fixed the numbers on my post above.

12

I’ll take care of creating the issues for this. :slightly_smiling_face:

2 Likes
13

@luisramos0 @sauloperez @kristinalim @Matt-Yorkley I would like to restart this conversation. We have been working with it for a while and I often feel an inner resistance when I use I18n.t in specs. Here are my reasons:

  • It’s harder to read.
  • Sometimes you need to add transformations or interpolations which add complexity. For example:
    I18n.t('spree.users.show.unconfirmed_email', unconfirmed_email: 'new@email.com')
    I18n.t('spree.admin.orders.form.line_item_adjustments').upcase
  • It replicates logic from the tested code and requires knowledge of the internal working which is bad practice in tests.
  • Moving I18n keys requires updating of specs.
  • Some typos like Welcome #{user.name} are not caught.

I also understand the upsides of using I18n.t:

  • Changing the text doesn’t require changing the spec. (Changing the text location or the interpolation variable does though.)
  • Easy interpolation when you want it:
    I18n.t(:card_has_been_removed, number: "x-#{default_card.last_digits}")

In my opinion, the advantages of using plain text often outweigh I18n.t. I’m wondering though if we really need a strict rule here. Maybe it depends on the situation which one is better. Is it really harmful to use both? What do you think?

14

I’m wondering though if we really need a strict rule here. Maybe it depends on the situation which one is better. Is it really harmful to use both? What do you think?

IMO that would make our job harder as we will need to take yet another decision while coding and we will discuss in every single PR whether A or B.

This case is not about an arbitrary stylistic choice so we need to weight the pros and cons and optimize for our use case in objective terms. So here is my evaluation:

It’s harder to read.

Impercetible difference but with the added benefit of the key path providing context to the reader.

Sometimes you need to add transformations or interpolations which add complexity. For example:
I18n.t('spree.users.show.unconfirmed_email', unconfirmed_email: 'new@email.com')
I18n.t('spree.admin.orders.form.line_item_adjustments').upcase

Yes, but again also a marginal difference. Besides, is this the common case? or just an exception?

It replicates logic from the tested code and requires knowledge of the internal working which is bad practice in tests.

Yes, you need to know the translation key, however, we do replicate logic very often in other tests and it’s not a problem.

Moving I18n keys requires updating of specs.

True, and we need to be aware of it. Is it something we do so often so that we need to optimize for it?

Some typos like Welcome #{user.name} are not caught.

I don’t really understand where is the typo in this one and where should be caught.

Changing the text doesn’t require changing the spec. (Changing the text location or the interpolation variable does though.)

The main reason why we choose this approach. So we need to check how often we change text location or interpolation vars (considering how many we have, which I think is very few) compared to its text.

So, IMO it’s about finding these numbers and taking a decision.