Issue 6231 - Add tests for translation string substitution

Issue 6231 - Add tests for translation string substitution

[Vasily Kuznetsov, 2018-01-08 17:39:19]

Hey Jon,

I got the .md part done here and it covers all scenarios from #6231.

We probably also need tests for translation string substitution in .html, .tmpl,
probably also for includes and templates. Maybe not as comprehensive as this
one, but at least the basics should be covered. We could also reuse the
translations file by adding a metadata header to those files (and then test that
feature as well, yay!). What do you think?

Cheers,
Vasily

https://codereview.adblockplus.org/29659605/diff/29659606/tests/expected_outp...
File tests/expected_output/en/translate (right):

https://codereview.adblockplus.org/29659605/diff/29659606/tests/expected_outp...
tests/expected_output/en/translate:8: <p>Entity escaping in links: <a
href="http://foo.com/?a&amp;b">Foo</a>.</p>
I don't think escaping HTML entities in URLs this way is particularly desirable,
but that's how it works at the moment and also how
https://issues.adblockplus.org/ticket/2457 says it should work. I guess we
should just roll with it for now.

[Vasily Kuznetsov, 2018-01-12 19:56:50]

Hi Jon,

Here's a final version of this patch. Now translations are tested quite well.
Looking forward to your comments!

Cheers,
Vasily

https://codereview.adblockplus.org/29659605/diff/29665555/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/29659605/diff/29665555/cms/sources.py#newc...
cms/sources.py:151: config = self.read_config()
Unfortunately I had to also make this substantive change -- without it
translation overrides only partially work. I hope nothing depends on this
partial working -- will need to test it before pushing this.

[Jon Sonesen, 2018-01-19 04:45:37]

Hey Vasily,

Thanks for adding more coverage and having the test pages be a bit more self
documenting.

Just the two comments, with the change on the translation api endpoint that
isn't a big deal, and it is simpler so not too worried about it.

Cheers!

https://codereview.adblockplus.org/29659605/diff/29665555/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/29659605/diff/29665555/cms/sources.py#newc...
cms/sources.py:151: config = self.read_config()
On 2018/01/12 19:56:49, Vasily Kuznetsov wrote:
> Unfortunately I had to also make this substantive change -- without it
> translation overrides only partially work. I hope nothing depends on this
> partial working -- will need to test it before pushing this.

Hm, I have played with it a fair amount and it seems legit. However, I have not
generated all of our sites and checked the locale string insertions for issues.

https://codereview.adblockplus.org/29659605/diff/29665555/tests/test_translat...
File tests/test_translations.py (right):

https://codereview.adblockplus.org/29659605/diff/29665555/tests/test_translat...
tests/test_translations.py:18: ('upload-translation?key=test_key&json=1',
'simple'),
Is this unrelated?

[Vasily Kuznetsov, 2018-01-19 15:34:29]

Here we go, some more stuff covered. Don't worry that it's a lot of files, they
are mostly just pages and expected outputs.

I promise I will now stop adding stuff to this review ;)

Cheers,
Vasily

https://codereview.adblockplus.org/29659605/diff/29665555/cms/sources.py
File cms/sources.py (right):

https://codereview.adblockplus.org/29659605/diff/29665555/cms/sources.py#newc...
cms/sources.py:151: config = self.read_config()
On 2018/01/19 04:45:37, Jon Sonesen wrote:
> On 2018/01/12 19:56:49, Vasily Kuznetsov wrote:
> > Unfortunately I had to also make this substantive change -- without it
> > translation overrides only partially work. I hope nothing depends on this
> > partial working -- will need to test it before pushing this.
> 
> Hm, I have played with it a fair amount and it seems legit. However, I have
not
> generated all of our sites and checked the locale string insertions for
issues.

Checked on help.eyeo.com, web.acceptableads.org, web.adblockplus.org,
web.acceptableads.com, web.adblockbrowser.org and web.eyeo.com. The results are
identical.

https://codereview.adblockplus.org/29659605/diff/29665555/tests/test_translat...
File tests/test_translations.py (right):

https://codereview.adblockplus.org/29659605/diff/29665555/tests/test_translat...
tests/test_translations.py:18: ('upload-translation?key=test_key&json=1',
'simple'),
On 2018/01/19 04:45:37, Jon Sonesen wrote:
> Is this unrelated?

This is needed because I renamed the translation string in `translate.md`.

https://codereview.adblockplus.org/29659605/diff/29674728/tests/test_page_out...
File tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/29659605/diff/29674728/tests/test_page_out...
tests/test_page_outputs.py:58: if expected_output.startswith('## MISSING'):
This allows us to check that a certain file _is_not_ generated.

https://codereview.adblockplus.org/29659605/diff/29674728/tests/test_page_out...
tests/test_page_outputs.py:67: assert expected_output == response.read().strip()
I changed the order of arguments in a comparison to make the diffs in error
messages look more natural.

[Jon Sonesen, 2018-01-19 19:25:25]

On 2018/01/19 15:34:29, Vasily Kuznetsov wrote:
> Here we go, some more stuff covered. Don't worry that it's a lot of files,
they
> are mostly just pages and expected outputs.
> 
> I promise I will now stop adding stuff to this review ;)
> 
> Cheers,
> Vasily
> 
> https://codereview.adblockplus.org/29659605/diff/29665555/cms/sources.py
> File cms/sources.py (right):
> 
>
https://codereview.adblockplus.org/29659605/diff/29665555/cms/sources.py#newc...
> cms/sources.py:151: config = self.read_config()
> On 2018/01/19 04:45:37, Jon Sonesen wrote:
> > On 2018/01/12 19:56:49, Vasily Kuznetsov wrote:
> > > Unfortunately I had to also make this substantive change -- without it
> > > translation overrides only partially work. I hope nothing depends on this
> > > partial working -- will need to test it before pushing this.
> > 
> > Hm, I have played with it a fair amount and it seems legit. However, I have
> not
> > generated all of our sites and checked the locale string insertions for
> issues.
> 
> Checked on http://help.eyeo.com, http://web.acceptableads.org,
http://web.adblockplus.org,
> http://web.acceptableads.com, http://web.adblockbrowser.org and
http://web.eyeo.com. The results are
> identical.
> 
>
https://codereview.adblockplus.org/29659605/diff/29665555/tests/test_translat...
> File tests/test_translations.py (right):
> 
>
https://codereview.adblockplus.org/29659605/diff/29665555/tests/test_translat...
> tests/test_translations.py:18: ('upload-translation?key=test_key&json=1',
> 'simple'),
> On 2018/01/19 04:45:37, Jon Sonesen wrote:
> > Is this unrelated?
> 
> This is needed because I renamed the translation string in `translate.md`.
> 
>
https://codereview.adblockplus.org/29659605/diff/29674728/tests/test_page_out...
> File tests/test_page_outputs.py (right):
> 
>
https://codereview.adblockplus.org/29659605/diff/29674728/tests/test_page_out...
> tests/test_page_outputs.py:58: if expected_output.startswith('## MISSING'):
> This allows us to check that a certain file _is_not_ generated.
> 
>
https://codereview.adblockplus.org/29659605/diff/29674728/tests/test_page_out...
> tests/test_page_outputs.py:67: assert expected_output ==
response.read().strip()
> I changed the order of arguments in a comparison to make the diffs in error
> messages look more natural.

Hey,

So pretty much looks good to me! Once you merge def wanna keep an eyeo out for
generation failures but I think this will be really good so thanks a ton. Also,
I will wait to update my other review on the translations tests until we get
this merged and verify all is good. 

Cheers!!
LGTM

[Jon Sonesen, 2018-01-19 19:25:35]

https://codereview.adblockplus.org/29659605/diff/29674728/tests/test_page_out...
File tests/test_page_outputs.py (right):

https://codereview.adblockplus.org/29659605/diff/29674728/tests/test_page_out...
tests/test_page_outputs.py:58: if expected_output.startswith('## MISSING'):
On 2018/01/19 15:34:29, Vasily Kuznetsov wrote:
> This allows us to check that a certain file _is_not_ generated.

Acknowledged.

https://codereview.adblockplus.org/29659605/diff/29674728/tests/test_page_out...
tests/test_page_outputs.py:67: assert expected_output == response.read().strip()
On 2018/01/19 15:34:29, Vasily Kuznetsov wrote:
> I changed the order of arguments in a comparison to make the diffs in error
> messages look more natural.

Acknowledged.