Noissue - Refactor translation tests to validate sync

Noissue - Refactor translation tests to validate sync

[Jon Sonesen, 2017-12-22 15:45:27]

On 2017/12/22 15:22:37, Jon Sonesen wrote:

So IMO the way I was testing before was more like a unit test by validating each
expected request with the api when what we really want to do is validate the
resulting locales structure with what is expected (what is returned in all.zip)
and what features we are checking out. Also using this method of validation I
can easily test the feature for #5600

[Vasily Kuznetsov, 2018-01-02 15:10:05]

Hi Jon,

I think this new approach is definitely better than what we had before. I have
some comments on the exact implementation though, see below. Let me know (by IRC
for example) if something is not clear, and we can chat and hammer out the
details.

Cheers,
Vasily

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

https://codereview.adblockplus.org/29647615/diff/29647625/tests/test_translat...
tests/test_translations.py:15: def api_zip(temp_site, request):
Am I right that the purpose of this fixture is to examine the zip-file that is
produced by running the translation script in line 36?

If this is indeed the case, I think the fixture is not the right approach.
Fixture is something we prepare before the test for the tested code (and
sometimes assertions) to use. However, here we want to examine the output of the
tested code (after it was run), so we should probably just put this code into
the test or make it a function.

https://codereview.adblockplus.org/29647615/diff/29647625/tests/test_translat...
tests/test_translations.py:38: assert locale in
os.listdir(os.path.join(temp_site, 'locales'))
I get an error here:

    def test_sync(temp_site, make_intercept, api_zip):
        sys.argv = [None, temp_site, 'test_key']
        runpy.run_module('cms.bin.translate', run_name='__main__')
        for locale in [os.path.dirname(x) for x in api_zip.namelist()]:
->          assert locale in os.listdir(os.path.join(temp_site, 'locales'))
E           AssertionError: assert '.' in ['de', 'en']

[Jon Sonesen, 2018-01-05 10:27:08]

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

https://codereview.adblockplus.org/29647615/diff/29647625/tests/test_translat...
tests/test_translations.py:15: def api_zip(temp_site, request):
On 2018/01/02 15:10:04, Vasily Kuznetsov wrote:
> Am I right that the purpose of this fixture is to examine the zip-file that is
> produced by running the translation script in line 36?
> 
> If this is indeed the case, I think the fixture is not the right approach.
> Fixture is something we prepare before the test for the tested code (and
> sometimes assertions) to use. However, here we want to examine the output of
the
> tested code (after it was run), so we should probably just put this code into
> the test or make it a function.

No, this generated the zips that the test validates

https://codereview.adblockplus.org/29647615/diff/29647625/tests/test_translat...
tests/test_translations.py:38: assert locale in
os.listdir(os.path.join(temp_site, 'locales'))
On 2018/01/02 15:10:04, Vasily Kuznetsov wrote:
> I get an error here:
> 
>     def test_sync(temp_site, make_intercept, api_zip):
>         sys.argv = [None, temp_site, 'test_key']
>         runpy.run_module('cms.bin.translate', run_name='__main__')
>         for locale in [os.path.dirname(x) for x in api_zip.namelist()]:
> ->          assert locale in os.listdir(os.path.join(temp_site, 'locales'))
> E           AssertionError: assert '.' in ['de', 'en']

odd, i dont, but will look into it

[Vasily Kuznetsov, 2018-01-05 14:30:16]

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

https://codereview.adblockplus.org/29647615/diff/29647625/tests/test_translat...
tests/test_translations.py:15: def api_zip(temp_site, request):
On 2018/01/05 10:27:07, Jon Sonesen wrote:
> On 2018/01/02 15:10:04, Vasily Kuznetsov wrote:
> > Am I right that the purpose of this fixture is to examine the zip-file that
is
> > produced by running the translation script in line 36?
> > 
> > If this is indeed the case, I think the fixture is not the right approach.
> > Fixture is something we prepare before the test for the tested code (and
> > sometimes assertions) to use. However, here we want to examine the output of
> the
> > tested code (after it was run), so we should probably just put this code
into
> > the test or make it a function.
> 
> No, this generated the zips that the test validates

Ok, I see. But then I'm not sure what this test is testing. It seems like it
does the following:
1. Generates a zip file from the `locales` folder of the test website (that's
`api_zip` fixture).
2. Runs `cms.bin.translate` (that gets the generated `zip` file from the mock
API -- this hidden dependency is not great, but it existed before).
3. Checks that the `locales` folder of the test website has all the directories
from the zip file.

If step 2 doesn't do anything at all, the test would still pass, because the zip
file was generated from the folder with which we compare it. Seems like we
haven't really tested much.

So I think to make this meaningful, we need to introduce some changes to the zip
file and make the mock API return other changes that would have some effects
that we can then test.

Or we could just keep it simple and only check that nothing crashes during
Crowdin sync. But then the previous logging setup that was checking that the
correct HTTP requests were made was better than nothing, so we should probably
keep it.

https://codereview.adblockplus.org/29647615/diff/29647625/tests/test_translat...
tests/test_translations.py:38: assert locale in
os.listdir(os.path.join(temp_site, 'locales'))
On 2018/01/05 10:27:07, Jon Sonesen wrote:
> On 2018/01/02 15:10:04, Vasily Kuznetsov wrote:
> > I get an error here:
> > 
> >     def test_sync(temp_site, make_intercept, api_zip):
> >         sys.argv = [None, temp_site, 'test_key']
> >         runpy.run_module('cms.bin.translate', run_name='__main__')
> >         for locale in [os.path.dirname(x) for x in api_zip.namelist()]:
> > ->          assert locale in os.listdir(os.path.join(temp_site, 'locales'))
> > E           AssertionError: assert '.' in ['de', 'en']
> 
> odd, i dont, but will look into it

I printed `api_zip.namelist()` in this test and it contains the current
directory for some reason:

['./', 'de/', 'en/', 'de/translate.json', 'en/global.json']

It's kind of dumb that `ZipFile` does this, but I guess we'd need to live with
it somehow to make the test work reliably.

[Jon Sonesen, 2018-02-17 10:35:14]

On 2018/01/05 14:30:16, Vasily Kuznetsov wrote:
>
https://codereview.adblockplus.org/29647615/diff/29647625/tests/test_translat...
> File tests/test_translations.py (right):
> 
>
https://codereview.adblockplus.org/29647615/diff/29647625/tests/test_translat...
> tests/test_translations.py:15: def api_zip(temp_site, request):
> On 2018/01/05 10:27:07, Jon Sonesen wrote:
> > On 2018/01/02 15:10:04, Vasily Kuznetsov wrote:
> > > Am I right that the purpose of this fixture is to examine the zip-file
that
> is
> > > produced by running the translation script in line 36?
> > > 
> > > If this is indeed the case, I think the fixture is not the right approach.
> > > Fixture is something we prepare before the test for the tested code (and
> > > sometimes assertions) to use. However, here we want to examine the output
of
> > the
> > > tested code (after it was run), so we should probably just put this code
> into
> > > the test or make it a function.
> > 
> > No, this generated the zips that the test validates
> 
> Ok, I see. But then I'm not sure what this test is testing. It seems like it
> does the following:
> 1. Generates a zip file from the `locales` folder of the test website (that's
> `api_zip` fixture).
> 2. Runs `cms.bin.translate` (that gets the generated `zip` file from the mock
> API -- this hidden dependency is not great, but it existed before).
> 3. Checks that the `locales` folder of the test website has all the
directories
> from the zip file.
> 
> If step 2 doesn't do anything at all, the test would still pass, because the
zip
> file was generated from the folder with which we compare it. Seems like we
> haven't really tested much.
> 
> So I think to make this meaningful, we need to introduce some changes to the
zip
> file and make the mock API return other changes that would have some effects
> that we can then test.
> 
> Or we could just keep it simple and only check that nothing crashes during
> Crowdin sync. But then the previous logging setup that was checking that the
> correct HTTP requests were made was better than nothing, so we should probably
> keep it.
> 
>
https://codereview.adblockplus.org/29647615/diff/29647625/tests/test_translat...
> tests/test_translations.py:38: assert locale in
> os.listdir(os.path.join(temp_site, 'locales'))
> On 2018/01/05 10:27:07, Jon Sonesen wrote:
> > On 2018/01/02 15:10:04, Vasily Kuznetsov wrote:
> > > I get an error here:
> > > 
> > >     def test_sync(temp_site, make_intercept, api_zip):
> > >         sys.argv = [None, temp_site, 'test_key']
> > >         runpy.run_module('cms.bin.translate', run_name='__main__')
> > >         for locale in [os.path.dirname(x) for x in api_zip.namelist()]:
> > > ->          assert locale in os.listdir(os.path.join(temp_site,
'locales'))
> > > E           AssertionError: assert '.' in ['de', 'en']
> > 
> > odd, i dont, but will look into it
> 
> I printed `api_zip.namelist()` in this test and it contains the current
> directory for some reason:
> 
> ['./', 'de/', 'en/', 'de/translate.json', 'en/global.json']
> 
> It's kind of dumb that `ZipFile` does this, but I guess we'd need to live with
> it somehow to make the test work reliably.

After our discussions in person, this will hold off since it isnt really testing
anything, but will be useful later for testing new upstreams. Closing.