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

Rspec Issue: Missing locales in one spec breaks another #173

Closed
lynnfaraday opened this issue Oct 20, 2012 · 10 comments
Closed

Rspec Issue: Missing locales in one spec breaks another #173

lynnfaraday opened this issue Oct 20, 2012 · 10 comments
Milestone

Comments

@lynnfaraday
Copy link

I'm having a weird error with the rspec tests in my app related to the I18n library. This is a Ruby app, but not Rails. I18n is version 0.6.1

If one of my tests calls I18n.t without first setting the load_path, it appears to somehow mess up subsequent tests that do actually set the load path. To illustrate this failure in a very simple manner, I created two specs that don't even use any application code:

(just showing the specs for brevity; can provide the full source on request)
--test1.rb--
 it "should fail if a translation is not found" do
      puts "Translation fail"
      I18n.t('test').should eq "translation missing: en.test"
 end

--test2.rb--
it "should translate hello to english" do
  create_locale file_for_testing() 
  I18n.load_path << Dir.pwd + "/en.yml"
  puts "Translate English"
  I18n.locale = :en
  I18n.t("hello").should eq "Hello world"
end

If I adjust the file names so that the "English" translation test runs first, everything's fine:

Translate English
.Translation fail 
...
2 examples, 0 failures

If I adjust the file names such that the "fail" translation test runs first, it causes the "English" test to fail too!

Translation fail
.Translate English
F

Failures:

  1) foo should translate hello to english
     Failure/Error: I18n.t("hello").should eq "Hello world"

       expected: "Hello world"
            got: "translation missing: en.hello"
   ...
    2 examples, 1 failure

Clearly the first spec is somehow messing things up for the second spec. But what? And why?

@tigrish
Copy link
Collaborator

tigrish commented Oct 22, 2012

I believe I've encountered a similar situation where the load_path has to be defined before any I18n.t methods are called. I can't quite remember the specifics, but I can look into it.

It seems likely that even though the load_path is set, the actual translation data itself isn't reloaded every time a .t is called (this makes sense), but I guess it should be reloading if the load_path changes.

In the mean time, making test1.rb look like this should work no matter what order the specs are run in :

 it "should fail if a translation is not found" do
  puts "Translation fail"
  create_locale file_for_testing() 
  I18n.load_path << Dir.pwd + "/en.yml"
  I18n.t('test').should eq "translation missing: en.test"
 end

@lynnfaraday
Copy link
Author

@tigrish - Your suggestion helped me work around the problem, so thank you. (Though actually I stubbed out I18n.t() rather than loading the fake locale files in every single spec). Still, seems like a bug to me. As you say, changing the load_path ought to reload the translations.

@knservis
Copy link

knservis commented Nov 4, 2012

I just had a quick look at the code in lib/i18n/config.rb and it seems to me that reloading the backend every time the load_path changes is tricky.The user of the library would need to assign to load_path an object that is a (new) subtype of Array, with a callback to reload the backend when the size or contents of the load_path change. I wonder if there is a simpler way as that feels a bit too much.

@lynnfaraday
Copy link
Author

@knservis - Well, I was presuming that the intent was that load_path could be called at any time. I got this idea from the documentation:

The backend will lazy-load these translations when a translation is looked up for the first time. This makes it possible to just swap the backend with something else even after translations have already been announced.

If that's wrong, and you're not supposed to be able to alter it after you've begun calling t(), the solution could be as simple as making that an explicit (and documented) behavior rather than a head-scratcher. For instance:

  • Raise an exception if you call t() before load_path is set
  • Raise an exception if you set load_path after t() has already been called (since the translations are already loaded).

@knservis
Copy link

knservis commented Nov 6, 2012

@lynnfaraday - If I understand the documentation you sent correctly, it says that the paths will not be loaded until they are needed so that you can swap the backend, not that they will be reloaded when updated. If it is necessary to update the paths after using them you could always reload them yourself, e.g.:

require_relative '../lib/i18n'

describe "Issue 173" do

 it "should fail when translation is not found in paths" do
   I18n.t('bar', :scope => 'foo').should eq "translation missing: en.foo.bar"
 end

 it "should translate bar to english" do
   I18n.load_path << File.expand_path('../../test/test_data/locales/en.yml', __FILE__)
   I18n.locale = :en
   I18n.backend.reload!
   I18n.t('bar', :scope => 'foo').should eq 'baz'
 end

end

I don't think that it is particularly difficult to make the backend reload itself when the paths are updated but it might imply changes to the way people use the this library and might add complexity and reduce readability for something that may be a rare use case. What do you think?

@radar
Copy link
Collaborator

radar commented Nov 3, 2016

Hi there,

It's been a while, but I don't suppose anyone has an app which reproduces this issue? That'd help tremendously in debugging this.

Thanks!

@radar radar added the no-repro label Nov 3, 2016
@lynnfaraday
Copy link
Author

@radar It's not an app, but here's a gist that illustrates the issue.

Just create the three files and run rspec.

rspec spec.rb works - both tests pass

rspec spec_failed.rb illustrates the problem - both tests fail

The only difference between the two spec files is the order of the tests.

@radar
Copy link
Collaborator

radar commented Nov 9, 2016

Thanks @lynnfaraday!

I've been able to investigate this today and I've discovered a solution. However, it doesn't work with I18n.load_path <<, only I18n.load_path +=. I think to make it work with the former we'd need to re-define the << method on load_path, which may be a bit hacky. tbh I'd rather advise people to use I18n.load_path +=.

@radar
Copy link
Collaborator

radar commented Nov 9, 2016

☝️ there's a PR which will fix your issue. Here's an updated Gist of the code I used to verify this patch: https://gist.github.com/radar/c74cadff6dee7988f764536d667f44df

@radar radar added this to the 0.8.0 milestone Nov 9, 2016
@radar
Copy link
Collaborator

radar commented Nov 20, 2016

Just merged #348 which fixes this. Thanks @lynnfaraday!

@radar radar closed this as completed Nov 20, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants