[Translator]Test case refactoring and simplifications for the Translator class #14291

mpdude · Apr 9, 2015

Q	A
Bug fix?	no
New feature?	no
BC breaks?	no
Deprecations?	no
Fixed tickets	~
Tests pass?	yes
License	MIT

Also see #14280 and #14281.

This also tests for the fix in b5da2ae87606e1f47081d6e6f09dd61ccb6ab501

aitboudad · Apr 9, 2015

src/Symfony/Component/Translation/Tests/TranslatorCacheTest.php

+
+        foreach (array(false, true) as $debug) {
+            // Prime the cache
+            $translator = new Translator($locale, null, $this->tmpDir);


missing debug argument

new Translator($locale, null, $this->tmpDir, $debug)

aitboudad · Apr 9, 2015

the failed test for dep=high is not related to this PR.

aitboudad · Apr 9, 2015

can you squash your commits into one, please

mpdude · Apr 11, 2015

Done.

linaori · Apr 11, 2015

This would be easier with a true/false provider in my opinion:

public function booleanProvider()
{
    return array(array(true), array(false));
}

/**
 * @dataProvider booleanProvider
 */
public function testSomething($debug)
{
}

Then you would just test it with true and with false instead of implementing a foreach in each method.

mpdude · Apr 11, 2015


~~~@aitboudad :-)?~~~

mpdude · Apr 11, 2015

As the @dataProvider approach also allows to remove the deleteTmpDir() call, it's a good thing(TM).

Thanks @iltar!

aitboudad · Apr 11, 2015

src/Symfony/Component/Translation/Translator.php

@@ -466,11 +466,12 @@ private function getFallbackContent(MessageCatalogue $catalogue)
    private function getCatalogueCachePath($locale)
    {
        $catalogueHash = sha1(serialize(array(
+            'locale' => $this->locale,


I'm not sure about it , if someone wants to manually refresh the cache for en catalogue for example ?

A simple translations cache warmup (that is now available) should clear all cached translations for which the Resources have changed/expired, right?

right, but locale is a catalog identifier and include it in catalogueHash makes difficult to identify catalog file.

aitboudad · Apr 12, 2015

@mpdude can you squash your commits :)

mpdude · Apr 13, 2015

Squashed.

aitboudad · Apr 14, 2015

src/Symfony/Component/Translation/Tests/TranslatorCacheTest.php


-        $this->assertEquals('foo A', $translator->trans('foo'));
+        $catalogue = new MessageCatalogue($locale, array());
+        $catalogue->addResource(new StaleResource()); // better use a helper class than a mock, because it gets serialized in the cache and re-loaded


we can use mock here the only thing that doesn't work as expected is expects method.

$resource = $this->getMock('Symfony\Component\Config\Resource\ResourceInterface'); $resource->method('isFresh')->will($this->returnValue(false)); catalogue->addResource($resource);

mpdude · Apr 14, 2015

Hm, sounds a bit whacky, doesn't it?

But if you prefer I'll change that.

aitboudad · Apr 14, 2015

@mpdude ok keep it as you like it 😄

aitboudad · Apr 14, 2015

src/Symfony/Component/Translation/Tests/TranslatorCacheTest.php

-
-        $this->assertEquals('foo B', $translator->trans('foo'));
+        // 1st pass
+        $translator = new Translator($locale, null, $this->tmpDir, true);


we can simplify below code by:

$translator = new Translator($locale, null, $this->tmpDir, true); $translator->addLoader($format, $loader); $translator->addResource($format, null, $locale); // 1st pass $translator->trans($msgid); // 2nd pass $translator->trans($msgid);

sorry forget it won't work as the catalogue already loaded

aitboudad · Apr 19, 2015

@mpdude I think we must apply these changes into 2.6, can you create a PR for 2.6 in favor of this ?

mpdude · Apr 21, 2015

@aitboudad Some of the methods I changed/refactored are not yet available in 2.6. Do we really need to backport this? (Or maybe even move it to 2.8 instead?)

stof · Apr 21, 2015

@mpdude keeping the same test structure between branches makes the maintenance much easier for us when merging between branches (avoiding conflicts). So test improvements should be applied to the older versions being meaningful

aitboudad · Apr 21, 2015

if some change are only relevant for 2.7 you can keep it in this PR, I already do the same thing in #14341 (comment) ;)

mpdude · Apr 22, 2015

See #14437. I'll probably need to rebase this one after #14437 is merged and makes it up into 2.7?

…or tests. (mpdude) This PR was squashed before being merged into the 2.6 branch (closes #14437). Discussion ---------- [2.6][Translator] Extend, refactor and simplify Translator tests. | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Fixed tickets | ~ | Tests pass? | yes | License | MIT These are the improvements from #14291 that we can easily backport to the 2.6 branch as requested [here](#14291 (comment)). Commits ------- a8c4471 [2.6][Translator] Extend, refactor and simplify Translator tests.

aitboudad · Apr 24, 2015

@mpdude you can rebase now

mpdude · Apr 24, 2015

@aitboudad Thanks for the heads-up

xabbuh · Apr 24, 2015

@mpdude Can you rebase once again (tests have been fixed in #14464)?

This are the remaining changes on the 2.7 branch after #14437 merged most of the improvements into 2.6.

mpdude · Apr 24, 2015

@xabbuh Done. (/cc @aitboudad)

aitboudad · Apr 25, 2015

Thank you @mpdude.

…r the Translator class (mpdude) This PR was merged into the 2.7 branch. Discussion ---------- [Translator]Test case refactoring and simplifications for the Translator class | Q | A | ------------- | --- | Bug fix? | no | New feature? | no | BC breaks? | no | Deprecations? | no | Fixed tickets | ~ | Tests pass? | yes | License | MIT Also see #14280 and #14281. This also tests for the fix in [b5da2ae](b5da2ae#diff-3126bfacb25e1e304b8cc720e8527a19) Commits ------- c0300f5 Remaining tweaks for translator tests on 2.7

aitboudad reviewed Apr 9, 2015
View reviewed changes

aitboudad added the Translation label Apr 9, 2015

aitboudad changed the title ~~Test case refactoring and simplifications for the Translator class~~ [Translator]Test case refactoring and simplifications for the Translator class Apr 9, 2015

aitboudad reviewed Apr 11, 2015
View reviewed changes

mpdude mentioned this pull request Apr 13, 2015

[2.3][Translation] test refresh cache when resources File change. #14279

Merged

aitboudad reviewed Apr 14, 2015
View reviewed changes

mpdude mentioned this pull request Apr 22, 2015

[2.6][Translator] Extend, refactor and simplify Translator tests. #14437

Closed

Remaining tweaks for translator tests on 2.7

c0300f5

This are the remaining changes on the 2.7 branch after #14437 merged most of the improvements into 2.6.

aitboudad merged commit c0300f5 into symfony:2.7 Apr 25, 2015

mpdude deleted the independent-translator-caches branch April 25, 2015 10:49

Search code, repositories, users, issues, pull requests...

Uh oh!

[Translator]Test case refactoring and simplifications for the Translator class #14291

[Translator]Test case refactoring and simplifications for the Translator class #14291

Uh oh!

Conversation

mpdude commented Apr 9, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aitboudad commented Apr 9, 2015

Uh oh!

aitboudad commented Apr 9, 2015

Uh oh!

mpdude commented Apr 11, 2015

Uh oh!

linaori commented Apr 11, 2015

Uh oh!

mpdude commented Apr 11, 2015

Uh oh!

mpdude commented Apr 11, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aitboudad commented Apr 12, 2015

Uh oh!

mpdude commented Apr 13, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mpdude commented Apr 14, 2015

Uh oh!

aitboudad commented Apr 14, 2015

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aitboudad commented Apr 19, 2015

Uh oh!

mpdude commented Apr 21, 2015

Uh oh!

stof commented Apr 21, 2015

Uh oh!

aitboudad commented Apr 21, 2015

Uh oh!

mpdude commented Apr 22, 2015

Uh oh!

aitboudad commented Apr 24, 2015

Uh oh!

mpdude commented Apr 24, 2015

Uh oh!

xabbuh commented Apr 24, 2015

Uh oh!

mpdude commented Apr 24, 2015

Uh oh!

aitboudad commented Apr 25, 2015

Uh oh!

Uh oh!