i18n_viz icon indicating copy to clipboard operation
i18n_viz copied to clipboard

Support dynamic external_tool_url. Fixes #21

Open atodorov opened this issue 8 years ago • 7 comments

@jhilden please review.

atodorov avatar Apr 18 '16 09:04 atodorov

I've updated the test to take into account the currently selected locale. You can either use parameters from the environment or I18n.locale (.default_locale). I've also added a sample translation file for Bulgarian, for completeness.

atodorov avatar Apr 19 '16 10:04 atodorov

@jhilden ping. Please see my previous comment about the monkey patch in the test. If we had rspec (and rspec-mocks) I could make this code look a bit cleaner by doing something like

allow(I18nViz::Middleware).to receive(:external_tool_url).and_return( lambda {} )

which essentially does the same as the monkey-patch behind the scenes. What do you think, which one would you prefer ?

atodorov avatar Apr 28 '16 20:04 atodorov

@atodorov I'm sorry that I didn't have time to take another bigger look at your code, it's certainly on my list.

concerning Rspec: I'm definitely not against switching to Rspec at all, I also prefer it nowadays, but I currently just don't have the time to do it.

jhilden avatar May 02 '16 09:05 jhilden

@jhilden ping. any update here, it's been a while. Are you still against the monkey-patch piece of code within the tests ?

atodorov avatar May 31 '17 19:05 atodorov

thanks for pinging me again @atodorov I totally forgot takeing another look at this.

I'm not strictly against the monkey patch within the tests, IF there is no easy other solution. So that would not be a blocker for me to merge the PR.

However, the tests seem to be failing now with:

I18nVizDynamicUrlTest#test_dynamic_external_url_with_Bulgarian_locale:
Selenium::WebDriver::Error::WebDriverError:  Unable to find Mozilla geckodriver. Please download the server from https://github.com/mozilla/geckodriver/releases and place it somewhere on your PATH. More info at https://developer.mozilla.org/en-US/docs/Mozilla/QA/Marionette/WebDriver.

I think this should be worked on.

jhilden avatar Jun 01 '17 06:06 jhilden

@jhilden the error looks like Travis wasn't able to install all the required gems although the log appears to show everything as installed before we run the tests. I'm pretty sure it is not related to the current changes but I can't help further ATM.

atodorov avatar Jun 01 '17 12:06 atodorov

@jhilden I've spent more time on this today. The selenium problem (cherry-picked here) and the HTML nested tags are fixed in #25 (I hope).

Now about the failures we're observing here:

  1) Failure:
I18nVizDynamicUrlTest#test_dynamic_external_url_with_Bulgarian_locale [/home/senko/i18n_viz/test/i18n_viz_test.rb:104]:
Expected false to be truthy.

  2) Failure:
I18nVizDynamicUrlTest#test_dynamic_external_url_with_English_locale [/home/senko/i18n_viz/test/i18n_viz_test.rb:98]:
Expected false to be truthy.

The assertion which fails is looking for the tooltip links when i18n_viz is enabled so I fixed it with

    assert page.has_css?(".i18n-viz")
    first('.i18n-viz').hover

atodorov avatar Jun 01 '17 21:06 atodorov