Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code

Issue 29647615: Noissue - Refactor translation tests to validate sync (Closed)

Created:
Dec. 22, 2017, 3:22 p.m. by Jon Sonesen
Modified:
Feb. 17, 2018, 10:35 a.m.
Reviewers:
Vasily Kuznetsov
Visibility:
Public.

Description

Noissue - Refactor translation tests to validate sync

Patch Set 1 : #

Total comments: 6
Unified diffs Side-by-side diffs Delta from patch set Stats (+12 lines, -33 lines) Patch
M tests/crowdin_mock_api.py View 2 chunks +1 line, -8 lines 0 comments Download
M tests/test_translations.py View 1 chunk +11 lines, -25 lines 6 comments Download

Messages

Total messages: 6
Jon Sonesen
Dec. 22, 2017, 3:22 p.m. (2017-12-22 15:22:37 UTC) #1
Jon Sonesen
On 2017/12/22 15:22:37, Jon Sonesen wrote: So IMO the way I was testing before was ...
Dec. 22, 2017, 3:45 p.m. (2017-12-22 15:45:27 UTC) #2
Vasily Kuznetsov
Hi Jon, I think this new approach is definitely better than what we had before. ...
Jan. 2, 2018, 3:10 p.m. (2018-01-02 15:10:05 UTC) #3
Jon Sonesen
https://codereview.adblockplus.org/29647615/diff/29647625/tests/test_translations.py File tests/test_translations.py (right): https://codereview.adblockplus.org/29647615/diff/29647625/tests/test_translations.py#newcode15 tests/test_translations.py:15: def api_zip(temp_site, request): On 2018/01/02 15:10:04, Vasily Kuznetsov wrote: ...
Jan. 5, 2018, 10:27 a.m. (2018-01-05 10:27:08 UTC) #4
Vasily Kuznetsov
https://codereview.adblockplus.org/29647615/diff/29647625/tests/test_translations.py File tests/test_translations.py (right): https://codereview.adblockplus.org/29647615/diff/29647625/tests/test_translations.py#newcode15 tests/test_translations.py:15: def api_zip(temp_site, request): On 2018/01/05 10:27:07, Jon Sonesen wrote: ...
Jan. 5, 2018, 2:30 p.m. (2018-01-05 14:30:16 UTC) #5
Jon Sonesen
Feb. 17, 2018, 10:35 a.m. (2018-02-17 10:35:14 UTC) #6
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.

Powered by Google App Engine
This is Rietveld