Refactor localization update test so people can just enable the test module to test

Hm.

My argument for providing the .po files as menu callbacks was actually that it would be possible to test the functions which actually download stuff from drupal.org. I think using the test module for manual tests isn't that useful, manual testing right now is only a problem because we don't have any real data to work with. That will change in the next months, I'm really not worried about that. I think people will want to test this with real data and real translations anyway :)

What happened since I suggested to do this is that guzzle got added to core, which has support for mocking HTTP requests ( It's an optional plugin that's not yet in core, but we can change that). We already started thinking about using that for the remote download tests, see #1447736-181: Adopt Guzzle library to replace drupal_http_request(). That would allow to make normal drupal.org requests in the remote download code, without any kind of local overrides or other hacks and guzzle would return what drupal.org would, as previously set up by the test. It would also allow to use DrupalUnitTestBase for these tests I think, which means fast tests! :)

I had a completely different idea right now that would not only help with testing but also when actually using it. In 7.x, l10n_update doesn't support dev releases, I guess that's still true now? Drupal allows to identify how recent a dev release is now, the version string looks like this: "7.x-1.0-alpha1+29-dev". We should be able to fall back to "7.x-1.0-alpha1" and download translations for that. That would allow people to download 8.x-dev as soon as an unstable/alpha/whatever release is out and download translations. Should we open an issue for this?

Falling back on a previous version .po file seems like a good idea to me, as does opening an issue for that.

Is the suggestion to make the test treat the test .po files it contains act as if the come from l.d.o?

And not to fool it into using the actual 7.x l.d.o po files?

So to be able to test with a .po file for Spanish for views, a 7.x po file would be manually downloaded from l.d.o, renamed and added to the test module? The views module .info file would have to be changed to a alpha (non dev) release too?

What about for manual testing or live usability testing?

For clarification, webchick and I wanted the same thing, but for a different reason. My reason was better automated test coverage, and as explained above, I think we have now better ways to do that with Guzzle.

The fallback stuff is of course a separate issue, that thought just occured to me while I was responding here and I wanted to write it down :)

:) #1862622: localization fallback so people can test translations with dev releases

I'll try to tackle this issue.

I have not investigated yet how Update modules made its test, as Berdir suggested in http://drupal.org/node/1804688#comment-6784366. That could be a starting point.

My thought was to replace the programatically generated modules, which is how the current test does it, by 'physical' modules. The attached patch is a POC for such a module. It adds a module locale_test_contrib_one which has a translation file on a remote url. Just like locale_test_translation is a module with local po file, locale_test_contrib_one is a module with remote po file. The patch is over a week old and my not apply, but perhaps it is of some help.

I'll look into your work. (Assigning the issue back to me.)

One of Berdir's suggestions was to provide an URL where a translation can be downloaded, instead of a 'physical' file as provided by locale_test_translation. I'm not sure if this will work with the time stamp that the download/import requires (locale_translation_http_check()) . If it works it can be used for all remote translation URL's. Needs investigation.

Some thought about the above patch. The functions and (timestamp) definitions required by this module can be moved to the locale_test. All locale_test_* modules can now use this code.
Each locale_test_* modules takes care of all changes required for this module to mock a contrib module or a custom module. It changes how it is listed (locale_translation_project_list()), sets the initial timestamp of the translation file and sets the URL where the remote translation is found.

Long IRC discussion. May be usefull to others who want to know more about the internals of Locale's po detection.

balintk: Sutharsan: hi, I'm working on http://drupal.org/node/1861360, and I got stucked a little bit.
[10:10pm] Druplicon: http://drupal.org/node/1861360 => Refactor localization update test so people can just enable the test module to test => Drupal core, locale.module, normal, active, 15 comments, 5 IRC mentions
[10:11pm] Sutharsan: balintk: tell me
[10:11pm] kalmanhosszu joined the chat room.
[10:14pm] Sutharsan: balintk: I guess you have a question for me...
[10:14pm] balintk: Sutharsan: so currently locale_test.module provides some project information (this was placed in your tamper module before) and LocaleUpdateTest creates po files for those fake projects. And the goal is to provide the po files from a module together with timestamps, so we're going to have a module which is more independent. Your patch contains a module that creates two po files, a local and a remote one, so it can be used as a real modul
[10:14pm] balintk:  we don't need to mockup. Did I get it right so far?
[10:16pm] Sutharsan: balintk: it is all mockup but to a different extend
[10:16pm] balintk: yes, sure
[10:18pm] Sutharsan: balintk: but the goal is to have test-facilities that have as little mystery as possible and can also be used in 'manual' tests.
[10:20pm] Sutharsan: balintk: we can go though the process now, perhaps we can come up with a new simple solution.
[10:20pm] balintk: Sutharsan: all right
[10:20pm] Sutharsan: balintk: I have not given it much though, except when I wrote this new code.
[10:21pm] Sutharsan: balintk: the first step is locale needs to know about the module
[10:21pm] Sutharsan: balintk: normal modules are not-hidden, test modules are hidden.
[10:22pm] balintk: Sutharsan: yes, so therefore we change this in hook_system_info_alter
[10:22pm] balintk: in locale_test.module
[10:22pm] Sutharsan: balintk: yes and for the human to uncomment one .info line
[10:23pm] balintk: Sutharsan: okay, so when humans want to use this module they just uncomment that line in the .info?
[10:24pm] Sutharsan: balintk: or they could enable locale_test which does it for them (using hook_system_info_alter)
[10:24pm] balintk: Sutharsan: but that's also hidden 
[10:24pm] Sutharsan: balintk: yep, need to unhide something
[10:25pm] balintk: tokay, clear
[10:25pm] balintk: s/tokay/okay
[10:25pm] Sutharsan: balintk: the different mock modules have functions in common, we could place that in one module, e.g. locale_test
[10:27pm] balintk: Sutharsan: different mock modules like locale_test_contrib_one, and we are going to need more?
[10:27pm] Sutharsan: balintk: the test uses five (or so)
[10:27pm] balintk: I see
[10:28pm] balintk: so e.g. the po file creation which is right now in locale_test_contrib_one could be refactored, right?
[10:28pm] Sutharsan: balintk: it needs to test different scenario's. The number of modules can be reduced a bit
[10:29pm] Sutharsan: balintk: well, the next thing is that modules need po files
[10:30pm] Sutharsan: balintk: A suggestion by Berdir was to use a URL to provide translation/po
[10:34pm] reyero left the chat room. (Quit: reyero)
[10:34pm] Sutharsan: balintk: I can't (yet) decide between physical files and URL
[10:34pm] balintk: Sutharsan: and I've also seen that he suggested using Guzzle, so that we could mimic real remote downloads
[10:36pm] Sutharsan: balintk: Guzzle is still 'young' in Drupal, I would not take that risk now
[10:36pm] balintk: Sutharsan: but let's take the physical files scenario in this walkthrough, so I can get the whole picture
[10:37pm] balintk: your initial patch attempts to create a remote and a local po file
[10:37pm] Sutharsan: balintk: for contrib modules we need a 'remote' po and, for some cases, a local po too
[10:37pm] webflo joined the chat room.
[10:38pm] Sutharsan: balintk: for custom modules we need only a 'local' po
[10:39pm] Sutharsan: balintk: the location of the remote file can be set by the module in its .info file (alternatively with hook_locale_translation_projects_alter)
[10:40pm] Sutharsan: balintk: but the local counterpart of this file is in the translations directory
[10:41pm] Sutharsan: balintk: contrib modules can also set the location of the po file in their .info file. (again alternatively with hook_locale_translation_projects_alter)
[10:41pm] Sutharsan: balintk: but each module can only set one path to a po file.
[10:46pm] Freso left the chat room. (Quit: Zzz...)
[10:47pm] Sutharsan: balintk: I think each mock module must have one physical po file
[10:48pm] Sutharsan: balintk: if a contrib module needs both a remote and local for the test, it will copy the file to the desired 'local' location
[10:48pm] balintk: Sutharsan: I'm following you so far, I have questions, but I'll save them for later
[10:48pm] Sutharsan: balintk: no, give 'm now
[10:48pm] • Sutharsan Feels like he is talking to himself
[10:48pm] DesireJM joined the chat room.
[10:49pm] balintk: Sutharsan: okay, so like in your patch, it copies the remote file to the local directory
[10:49pm] balintk: Sutharsan: no, not at all, I'm listening very carefully 
[10:49pm] balintk: Sutharsan: you can't imagine how useful this is for me
[10:50pm] Haza` is now known as Haza`Aw.
[10:50pm] balintk: Sutharsan: so, maybe it's just a small thing, I don't know what status your patch is in, but it tries to copy a remote translation file, but that doesn't exist
[10:50pm] balintk: Sutharsan: is it because that part is only POC yet?
[10:51pm] Sutharsan: balintk: don't know about the state of the patch
[10:52pm] Sutharsan: balintk: use what you can. I tried the mocking it could need.
[10:52pm] balintk: Sutharsan: okay, so it probably misses that po file, no problem
[10:53pm] balintk: Sutharsan: okay, so we have mock modules with remote and/or local po files. What's next?
[10:53pm] Sutharsan: balintk: getting Local to know about the location of the po file
[10:54pm] Sutharsan: balintk: .info or this alter
[10:55pm] Sutharsan: balintk: the problem is the domain in case of a remote po. The domain is equal to the domain of the drupal site. So it can not be hardcoded in the .info
[10:56pm] balintk: Sutharsan: I understand, yes
[10:56pm] Sutharsan: balintk: the alter hook should take care of this
[10:56pm] balintk: yes, I've found it
[10:57pm] Sutharsan: balintk: and there is also a default server pattern
[10:57pm] Sutharsan: balintk: contrib modules usually don't define a server pattern, so the default is used.
[10:57pm] balintk: Sutharsan: which is for l.d.o I guess
[10:58pm] Sutharsan: balintk: yep
[10:58pm] webflo1 joined the chat room.
[10:58pm] Sutharsan: balintk: but for the mock it should be set to the domain of the drupal (test) site
[10:58pm] balintk: Sutharsan: sure, it's clear
[10:59pm] Sutharsan: balintk: the path of the pattern can be set to anything, but for testing it is best to use all available parameters
[10:59pm] balintk: Sutharsan: I can see that in you patch: $url . '/%core/%project/%project-%version.%language._po
[10:59pm] balintk: *your
[10:59pm] Sutharsan: balintk: i.e %core, %project, %version, %language
[11:00pm] Sutharsan: balintk: the _po is a trick to bypass the .htaccess which block .po extensions
[11:00pm] balintk: Sutharsan: yes, you have explained that well in your comments
[11:02pm] webflo2 joined the chat room.
[11:02pm] balintk: Sutharsan: do we also need to tell Locale where the local po files are?
[11:02pm] webflo1 left the chat room. (Read error: Operation timed out)
[11:02pm] balintk: or only the remotes?
[11:03pm] Sutharsan: balintk: local files are not served by apache
[11:03pm] Sutharsan:
[11:03pm] Sutharsan: balintk: and therefore .htaccess is not relevant
[11:03pm] balintk: sure, so we can use the regular .po extension
[11:05pm] Sutharsan: balintk: if we serve po files with a menu callback (Berdir's suggestion) we can use regular .po files as source. That is less confusing.
[11:05pm] balintk: but I meant that if we need to specify the local translation file path in the .info file or in a hook? But I realized that we don't, since the project/translations folder will be used.
[11:07pm] Sutharsan: balintk: and for contrib modules you don't need to specify in the .info, we use the default server pattern config() variable
[11:07pm] balintk: right, you've said that
[11:07pm] balintk: so now Locale know about our po files, what's next?
[11:08pm] balintk: *knows
[11:08pm] Sutharsan: balintk: next is a time stamp
[11:10pm] Sutharsan: balintk: for local files the creation time is used, for remote files the 'last-modified' from the http header is used.
[11:11pm] Sutharsan: balintk: for the tests, this time stamp must be set to a define value
[11:11pm] Sutharsan: balintk: *for automated tests
[11:12pm] Sutharsan: balintk: for manual tests the user can edit the file and with that change the time stamp
[11:13pm] Sutharsan: balintk: for manual test using a 'remote' translation there is no need to manipulate that. As if you could change the file on l.d.o.
[11:15pm] balintk: Sutharsan: with the menu callback, we could manipulate the last-modified header for remote translations as well
[11:15pm] Sutharsan: balintk: for manual testing it is important to document in the 'remote' po file how it is used. And for example why the file should not be moved.
[11:17pm] Sutharsan: balintk: In the current automated tests each po file of each module has a distinct time stamp set, which does not change during the test process.
[11:18pm] Sutharsan: balintk: all time stamps are set relative to the current time (REQUEST_TIME, REQUEST_TIME - 100, -200, -300, -400)
[11:19pm] balintk: Sutharsan: yes, I found them; they are prefixed with 'now', 'new', 'medium', 'old'
[11:20pm] Sutharsan: balintk: yep
[11:20pm] balintk: Sutharsan: and all the po files have the same content in the current test
[11:21pm] balintk: Sutharsan: oh, almost, the actual translations are different
[11:21pm] Sutharsan: balintk: I have not mentioned the project name, version (optional !) each module has. But those can be set in the .info
[11:22pm] Sutharsan: balintk: yes, the translations are different to identify overwriting of translations
[11:23pm] Sutharsan: balintk: I think that covers everything
[11:23pm] balintk: Sutharsan: sorry, I didn't get this project name and version part
[11:24pm] boztek joined the chat room.
[11:25pm] Sutharsan: balintk: in order to construct a URL or URI where the po file can be found
[11:25pm] balintk: Sutharsan: oh, okay
[11:25pm] Sutharsan: balintk: the placeholders in /%core/%project/%project-%version.%language._po are replaced
[11:26pm] Sutharsan: balintk: %core is 8.x (9.x, etc)
[11:26pm] balintk: get it 
[11:26pm] balintk: Sutharsan: so that would be it, from this point the test class will do its job, right?
[11:27pm] Sutharsan: balintk: %project is either the 'project' set by the d.o packaging script for remote files or 'interface translation project' for contrib modules
[11:28pm] Sutharsan: balintk: %version is set in the .info, but is optional (often custom modules don't set a version)
[11:28pm] DesireJM left the chat room. (Quit: Computer has gone to sleep.)
[11:28pm] Sutharsan: balintk: if the test class and the settings in the modules match, yes.
[11:29pm] Sutharsan: balintk: that is a point of 'magic' which we should document. One part happens in the module, the other part in the test class
[11:30pm] Sutharsan: balintk: if they get out of sync, the test will fail
[11:31pm] balintk: Sutharsan: yes, I understand
[11:32pm] Sutharsan: balintk: I think it is good to have each module serve it's remote po file via a hook_menu url
[11:33pm] balintk: Sutharsan: I agree. But only the remote po?
[11:33pm] Sutharsan: balintk: a remote url is a black box any way
[11:33pm] Sutharsan: balintk: yes only the remote
[11:34pm] Sutharsan: balintk: if it requires a local file too, it copies the po file to the right location
[11:35pm] Sutharsan: balintk: the physical po file is used as source for the menu callback and to copy the local file from
[11:35pm] Sutharsan: balintk: a local file is nothing more as a previously downloaded remote file.
[11:35pm] balintk: Sutharsan: so when a mock module provides a local po it should copy one from tests/translations?
[11:36pm] Sutharsan: balintk: that is, for a mock contrib module
[11:36pm] balintk: ok
[11:36pm] Sutharsan: balintk: not for a mock custom module
[11:37pm] Sutharsan: balintk: the mock custom module is already there: locale_test_translate
[11:37pm] Sutharsan: balintk: perhaps rename it to make the difference clear
[11:38pm] balintk: Sutharsan: that module currently uses a remote po, doesn't it?
[11:38pm] Sutharsan: balintk: what is missing is the time stamp manipulation in this module
[11:38pm] balintk: it's defined in its .info file
[11:38pm] Sutharsan: balintk: nope. in the .info it says: interface translation server pattern = core/modules/locale/tests/test.%language.po
[11:39pm] balintk: right
[11:40pm] tanarurkerem left the chat room. (Quit: tanarurkerem)
[11:40pm] Sutharsan: balintk: I think the 'externals' of a mock module must be as realistic as possible. i.e. po file and .info file
[11:41pm] Sutharsan: balintk: the internals should be documented magic. i.e. time stamp, serving remote po, default server pattern override
[11:42pm] torthu is now known as torthu_away.
[11:42pm] balintk: Sutharsan: sounds good, yes

I can summarize that IRC discussion. @Sutharsan was teaching me about how Locale module detects translation files, what the different scenarios are and so forth. In the end he came to the conclusion that we should incorporate @Berdir's menu callback idea for testing remote translation files, but keep "physical" po files for testing local translation files, so that we stick to the reality as much as possible.

I'm on this issue and probably deliver a patch by the end of the weekend. Sorry for the delay.

Here is the work I've done so far. The patch doesn't change anything yet in the current tests, it only adds a new mock contrib module and helper functions. I would love to hear some feedback, and if you find this approach good, my plan is to create two more mock contrib modules and one custom. They will be very similar to the existing one, but will ship with different po files with different timestamps sticking to the scenarios in the current working tests.

I had a discussion with @chx on IRC, I've changed the event subscriber for working around the 'Last-Modified' HTTP header override issue based on his suggestions. So now it adds a new subscriber instead of overriding an existing one.

@Gábor: FinishResponseSubscriber doesn't come from Symfony, so I guess we can improve it to at least respect HTTP headers when they are already set and then we could dismiss this trickery with our subscriber and make the test module way more understandable.

@Gábor: Agreed, so how about opening an issue (I can take care of this later) for fixing it in FinishResponseSubscriber and remove the trickery from here once that fix is committed, but leave the event subscriber here until then to be able to run successful tests?

Generally looks good, looking forward to the next step. I think you should optimize for understandability of the code.

+++ b/core/modules/locale/tests/modules/locale_test/lib/Drupal/locale_test/LocaleTestBundle.phpundefined
@@ -0,0 +1,31 @@
+ * This is a workaround for not letting FinishResponseSubscriber to neglect and
+ * override the already configured 'Last-Modified' HTTP header which is set and

Write positive sentences, double negative is much harder to understand. e.g. This is a workaround to make sure the 'Last-Modified' HTTP header is not overridden by FinishResponseSubscriber ...

+++ b/core/modules/locale/tests/modules/locale_test/locale_test.moduleundefined
@@ -25,6 +25,60 @@ function locale_test_system_info_alter(&$info, $file, $type) {
+ * @param int $timestamp
+ *   Optional. Timestamp will be set as the creation date for the copy of
+ *   the translation file.

Add: By default the timestamp is the current time.

+++ b/core/modules/locale/tests/modules/locale_test/locale_test.moduleundefined
@@ -25,6 +25,60 @@ function locale_test_system_info_alter(&$info, $file, $type) {
+function locale_test_copy_translation_file($module, $translation_file, $timestamp = NULL) {
...
+function locale_test_serve_translation_file($module, $langcode, $timestamp) {
+  module_load_include('translation.inc', 'locale');
+  module_load_include('compare.inc', 'locale');
+
+  $projects = locale_translation_get_projects(array($module));
+  $source = locale_translation_source_build($projects[$module], $langcode);
+  $file = locale_translation_source_check_file($source);
+  $headers = array(
+    'Content-Type' => 'text/x-po; charset=utf-8',
+    'Last-Modified' => gmdate(DATE_RFC1123, $timestamp),
+  );
+  return file_transfer($file->uri, $headers);
+}

This seems like a very complex way to build the file uri. While at the same time locale_test_copy_translation_file() depends on the po file to be in the module's directory. And I think you can do the same here too.

+++ b/core/modules/locale/tests/modules/locale_test/locale_test.moduleundefined
@@ -25,6 +25,60 @@ function locale_test_system_info_alter(&$info, $file, $type) {
+    // Set desired timestamp as creation date for the copied translation file.

Difficult sentence. Suggestion: 'Set the timestamp of the newly copied file.' or 'Use the timestamp as the creation date of the copied file.'

+++ b/core/modules/locale/tests/modules/locale_test/locale_test.moduleundefined
@@ -25,6 +25,60 @@ function locale_test_system_info_alter(&$info, $file, $type) {
+/**
+ * Helper function to serve a translation file as a result of a page callback.
+ *

Are there really helper function? Just describe what it does.

+++ b/core/modules/locale/tests/modules/locale_test_contrib_one/locale_test_contrib_one.installundefined
@@ -0,0 +1,14 @@
+/**
+ * Implements hook_install().
+ */

Needs more docs. It is important to understand what happens here. A crucial action of the module.

+++ b/core/modules/locale/tests/modules/locale_test_contrib_one/locale_test_contrib_one.moduleundefined
@@ -0,0 +1,49 @@
+/**
+ * @file
+ * Mocks a custom module with local and remote po files.
+ */

How about defining all module properties here as constants. Local po timestamp, remote po timestamp and perhaps even module name, version, language code, and perhaps even local po file name, and remote po filename. In this way the only difference between locale_test_contrib_one and *_two and *_three would be a set of definitions at the start of the module.

Add: By default the timestamp is the current time.

Just want to add a link for reference in terms of the comment about adding default to the comments.
http://drupal.org/node/1354#functions
requires it. It is not just Sutharsan being meticulous. :)
It also specifies that optional should be in paren's ().

@balintk Did you have any intermediate work to post back?

I came back from vacation today, had been away from my computer during the holidays. So no progress for a while, but I'm eager to continue, only that I probably won't have time before the weekend. If that's too late, someone can take the issue over, otherwise I will be happy to proceed, especially after this great feedback! :)

Thank you very much for the fantastic feedback. I went through all your suggestions; improved comments, simplified some code, introduced constants everywhere. I really like the idea of constants.

This is what I had time for during the weekend. The next step is producing two new contrib and one custom mockup module which will be almost identical with the existing one, but with the help of constants, the differences can be defined easily. After that, reworking LocaleUpdateTest. I'll continue working on these tasks this the week. Thank you for your patience.

@balintk, Nice work. Most is of my comments are on documentation. The purpose of the module is to provide some magic, so we should be clear about what is does and why.

+++ b/core/modules/locale/tests/modules/locale_test/locale_test.moduleundefined
@@ -5,6 +5,9 @@
  * Implements hook_system_info_alter().

@@ -14,7 +17,7 @@
 function locale_test_system_info_alter(&$info, $file, $type) {
   // Only modify the system info if required.
   // By default the locale_test modules are hidden and have a project specified.
-  // To test the module detection proces by locale_project_list() the
+  // To test the module detection process by locale_project_list() the
   // test modules should mimic a custom module. I.e. be non-hidden.
   if (state()->get('locale.test_system_info_alter')) {
     if ($file->name == 'locale_test' || $file->name == 'locale_test_translate') {

locale_test module should unhide all locale_test_* modules.

+++ b/core/modules/locale/tests/modules/locale_test/locale_test.moduleundefined
@@ -25,6 +28,55 @@ function locale_test_system_info_alter(&$info, $file, $type) {
+  $translation_file_path = drupal_get_path('module', $module) . '/' . $translation_file;
+  $translate_directory = variable_get('locale_translate_file_directory', conf_path() . '/files/translations');
+  $destination = $translate_directory . '/' . $translation_file;

Variable names are inconsistent and not proper English. Use $translation_directory instead of $translate_directory

+++ b/core/modules/locale/tests/modules/locale_test/locale_test.moduleundefined
@@ -25,6 +28,55 @@ function locale_test_system_info_alter(&$info, $file, $type) {
+function locale_test_serve_translation_file($module, $translation_file, $timestamp) {
+  $translation_file_path = drupal_get_path('module', $module) . '/' . $translation_file;
+  $headers = array(
+    'Content-Type' => 'text/x-po; charset=utf-8',
+    'Last-Modified' => gmdate(DATE_RFC1123, $timestamp),
+  );
+  return file_transfer(drupal_realpath($translation_file_path), $headers);
+}

The use of drupal_realpath() is discouraged. Is it needed here? If so, add some comment.

+++ b/core/modules/locale/tests/modules/locale_test_contrib_one/locale_test_contrib_one.infoundefined
@@ -0,0 +1,8 @@
+;;hidden = TRUE
+dependencies[] = locale_test

Add some comment explaining to disable the 'hidden = TRUE' line for manual testing.

+++ b/core/modules/locale/tests/modules/locale_test_contrib_one/locale_test_contrib_one.installundefined
@@ -0,0 +1,16 @@
+/**
+ * @file
+ * Sets up local po file.
+ */

Add some comment here to describe the concepts and the purpose of this module. And how to use it in manual testing.

+++ b/core/modules/locale/tests/modules/locale_test_contrib_one/locale_test_contrib_one.moduleundefined
@@ -0,0 +1,61 @@
+ * The po file supplied with the module is used as mock remote translation file
+ * as well as a local po file. Because the interface translation server pattern
+ * contains the host name of this site it is defined here and not in the .info
+ * file.

This is magic. But looking at the .info file one would think the default remote po path will be used. Add some comment in the .info file which points to this function.

+++ b/core/modules/locale/tests/modules/locale_test_contrib_one/locale_test_contrib_one.moduleundefined
@@ -0,0 +1,61 @@
+  $url = url(NULL, array('absolute' => TRUE)) . 'locale-test-remote-translations';

Use a constant for this magic string: 'locale-test-remote-translations'

+++ b/core/modules/locale/tests/modules/locale_test_contrib_one/locale_test_contrib_one.moduleundefined
@@ -0,0 +1,61 @@
+  $items['locale-test-remote-translations/8.x/' . LOCALE_TEST_CONTRIB_ONE_PO_FILE] = array(

Don't hard code '8.x' use DRUPAL_CORE_COMPATIBILITY instead.

+++ b/core/modules/locale/tests/modules/locale_test_contrib_one/locale_test_contrib_one.moduleundefined
@@ -0,0 +1,61 @@
+ * Page callback; serves remote translation file.

Use ':' instead of ';'. Serves instead of serves.

+++ b/core/modules/locale/tests/modules/locale_test_contrib_one/locale_test_contrib_one.moduleundefined
@@ -0,0 +1,61 @@
+function locale_test_contrib_one_remote_translation() {
+  // This is essentially the exact same translation file as the one used for
+  // locale po file testing, but we serve its content through this callback.

The comment can be moved to the function's comment. Use less words (don't make me think (tm) )e.g. 'essentially the exact same' is 'the same'.

@Sutharsan: thanks for the thorough review again. I incorporated all your suggestions into this new patch.

The use of drupal_realpath() is discouraged. Is it needed here? If so, add some comment.

file_transfer() needs the absolute local filesystem path, and I've seen that drupal_realpath() is used in locale_translation_source_check_file(), that's why I decided to do the same here. Do you have a better suggestion?

Add some comment here to describe the concepts and the purpose of this module. And how to use it in manual testing.

You referred to the .install file, but I added the comments to the .module file, because I think most people would look at that file first.

I have forgotten this one:

locale_test module should unhide all locale_test_* modules

If there is no alternative for drupal_realpath() then, add a line of comment. But file_transfer() has been removed, see http://drupal.org/node/1957078. So it's back to the drawing board ;)

Back to needs work, and for #35 too.

As said before, @webchick and I had different reasons to do this issue.

As far as I understand, we now support 8.x translation downloads without any test modules, you can just pick the language and it will get whatever is there. That, IMHO, invalidates the reason @webchick had for this issue.

My reason is being able to provide better test coverage for this functionality, which we now should be able to do using a mocked guzzle client, once the guzzle conversion is through. That should be possible using fast DUBT tests, I'm not actually sure if it's possible to mock guzzle requests on drupalGet()/Post requests.

Changing assignment after talking with @balinkt in IRC.

@Berdir

As far as I understand, we now support 8.x translation downloads without any test modules, you can just pick the language and it will get whatever is there.

Since there is no 8.x release, and we dont have fallback, adding a language does not find any translation file to load.

... your #1 and my #9 speak to still wanting this easy to enable module, right?

Re-roll of #34 with required changes from .info to .info.yml

+++ b/core/modules/locale/tests/modules/locale_test/lib/Drupal/locale_test/LocaleTestBundle.phpundefined
@@ -0,0 +1,31 @@
+  /**
+   * Overrides Symfony\Component\HttpKernel\Bundle\Bundle::build().
+   */
+  public function build(ContainerBuilder $container) {

overrides and implements are {@inheritdoc} now.

http://drupal.org/node/1354#inheritdoc

+++ b/core/modules/locale/tests/modules/locale_test/lib/Drupal/locale_test/LocaleTestFinishResponseSubscriber.phpundefined
@@ -0,0 +1,58 @@
+ * Definition of Drupal\locale_test\LocaleTestFinishResponseSubscriber.

This is Contains and full path with \ in all comments for namespaces.

+++ b/core/modules/locale/tests/modules/locale_test/lib/Drupal/locale_test/LocaleTestFinishResponseSubscriber.phpundefined
@@ -0,0 +1,58 @@
+  public $lastModified;

I added a @var block.. is it a string?

+++ b/core/modules/locale/tests/modules/locale_test/lib/Drupal/locale_test/LocaleTestFinishResponseSubscriber.phpundefined
@@ -0,0 +1,58 @@
+   * Saves the 'Last-Modified' HTTP header in a class property.
+   */
+  public function saveLastModified(FilterResponseEvent $event) {
...
+   * Sets the 'Last-Modified' HTTP header to the response.
+   */
+  public function setLastModified(FilterResponseEvent $event) {

missing @param

+++ b/core/modules/locale/tests/modules/locale_test_contrib_one/locale_test_contrib_one-8.x-1.1.de.poundefined
--- /dev/null
+++ b/core/modules/locale/tests/modules/locale_test_contrib_one/locale_test_contrib_one.info.ymlundefined

+++ b/core/modules/locale/tests/modules/locale_test_contrib_one/locale_test_contrib_one.info.ymlundefined
--- /dev/null
+++ b/core/modules/locale/tests/modules/locale_test_contrib_one/locale_test_contrib_one.installundefined

+++ b/core/modules/locale/tests/modules/locale_test_contrib_one/locale_test_contrib_one.moduleundefined
@@ -0,0 +1,74 @@
+ * Provides a callback for the remote translation file.
...
+ * Page callback: Serves remote translation file.

mmm callbacks.
check into #1971384: [META] Convert page callbacks to controllers

======
why are all these -do-not-test.patch ?

PAtch re-rolled.

Oops, re-rolled the wrong patch :(
Re-roll of #43.

Re-roll patch #46

After many years I'm circling back to locale module.

It is almost 5 years since serious work was done on this issue. Although the patch still applies, the API it uses is partly gone and the architecture is very outdated. Still I see the need to improve the way we mock modules and translations for interface translation tests.

Issue summary:

@webchick suggested to make the test module more like a standalone test system which you can enable and see working like update.module's test.

#38 and #40:

As far as I understand, we now support 8.x translation downloads without any test modules, you can just pick the language and it will get whatever is there. That, IMHO, invalidates the reason @webchick had for this issue.

This use case has become less demanding since translations files for both stable and dev releases are available. But when developing off-line and when developing tests there is still a demand for an easy way to mock modules and their translations. The current approach is backed into the test script which makes it hard to reuse outside the tests and hard to make changes or additions.

@berdir in #38:

My reason is being able to provide better test coverage for this functionality, which we now should be able to do using a mocked guzzle client, once the guzzle conversion is through. That should be possible using fast DUBT tests, I'm not actually sure if it's possible to mock guzzle requests on drupalGet()/Post requests.

As far as I understand, Guzzle's MockPlugin can only be used with the requests from Guzzle's Client. An other library like InterNations/http-mock could be used to mock any http response. But mocking the http reponse is probably only needed for unit test. When the full Drupal stack is available, a Drupal response can be used too.

My plan is to make a module that uses a yaml type plugin to register mock projects. The module serves as a project repository. Mock projects can be provides on demand. Each project can have a local and a remote translation source file. The local source file is copied (on demand) to the translations:// directory. The remote source file is made available (on demand) via a dynamic route.

This patch contains a module locale_test_mock that provides project and translation mock files which can be enabled via by code from within the tests. More details can be found in the README.txt.

I am currently looking for a review at conceptual and architectural level, as well as DX feedback.

No interdiff provided as this is a completely new start.