Translate Test Cases Strings should be LOWER PRIORITY than other UI strings.

I did a quick analysis of translatable strings in Test Cases (*Test.php). Drupal 8 has about 35xx strings. Remove the duplicated strings in non-testcases files, I think there's about 500 ~ 2000 strings. Usually, only senior developers would access Tests modules. As my own experiences, I even won't read the failing test messages and debugging / find out the problem in source code directly.

We should find a way to filter / mark these strings in Translation UI to help translator make decision.

** Remember that it also apply to all modules, not only the CORE. There're can be 10,000 test cases strings.

CommentFileSizeAuthor
#13 2600816.patch574 bytesgábor hojtsy

Comments

droplet created an issue. See original summary.

droplet’s picture

Issue summary: View changes
gábor hojtsy’s picture

Status: Active » Postponed (maintainer needs more info)

The localization server does not have a concept of prioritized strings vs. other strings, so to solve this that way would be quite a stretch.

Why do tests translate strings that do not otherwise show up on the Drupal UI as translated to begin with? Things like getInfo(), assert messages, etc. are not supposed to be translated. The coding standards at https://www.drupal.org/node/325974 for some reason do not explain the assert messages convention but it does the getInfo().

Can you list representative examples of those 500 - 2000 strings (that is a pretty wide range?).

droplet’s picture

Status: Postponed (maintainer needs more info) » Needs work

It would shown up on UI but an icon or whatever marked it as Test Case Strings.

Can you list representative examples of those 500 - 2000 strings (that is a pretty wide range?).

Sorry. I'd do more deeply analysis soon but if it's banned at beginning. I'm better to save my time to do more translation instead. We have very limited resources. I trying anything can max our efforts.

gábor hojtsy’s picture

Test cases should not have translatable strings which are not showing up in Drupal code itself (outside of tests). Implementing this feature on the localization server side is not the solution. Why are tests using such strings, care to answer my question?

droplet’s picture

$this->assertText(t('Updates for: @project', array('@project' => t('Drupal core'))), 'Translations found');
$this->assertRaw(t('@post %title has been created.', array('@post' => 'Basic page', '%title' => $edit['title[0][value]'])));
Test cases should not have translatable strings which are not showing up in Drupal code itself (outside of tests).

It's a good question. If so why to show it in Translation UI ?

droplet’s picture

Only exist in Test Case

#: core/modules/node/src/Tests/NodeRSSContentTest.php:48
msgid "Extra data that should appear only in the RSS feed for node @nid."
msgstr ""

#: core/modules/node/src/Tests/NodeRSSContentTest.php:53
msgid "Extra data that should appear everywhere except the RSS feed for node @nid."
msgstr ""

#: core/modules/node/src/Tests/NodeRSSContentTest.php:57
msgid "Value of testElement RSS element for node @nid."
msgstr ""

Also in other UI

#: core/modules/node/src/NodeTypeListBuilder.php:63; core/modules/node/src/Tests/NodeTypeTest.php:231
msgid "No content types available. <a href=\":link\">Add content type</a>."
msgstr ""
droplet’s picture

Assumed that ALL should not be translatable strings. Even Core Development we have reviewers & committer to scan every patches we missed it. Shouldn't we do something for contributed modules also ?

gábor hojtsy’s picture

This is a great example. Whatever it asserts would ALSO be t()-ed in Drupal itself, that is what is being tested after all. The assert message 'Translations found' however is not translated. Not seeing anything in this example or the other example you posted that is not t()-ing things that should already be t()-ed in Drupal's code.

$this->assertText(t('Updates for: @project', array('@project' => t('Drupal core'))), 'Translations found');

Looking at the concrete strings, these do seem to be not testing what Drupal actually generates:

$ git grep "Updates for:"
core/modules/locale/src/Tests/LocaleUpdateInterfaceTest.php:    $this->assertText(t('Updates for: @modules', array('@modules' => 'Locale test translate')), 'Translations available');
core/modules/locale/src/Tests/LocaleUpdateInterfaceTest.php:    $this->assertText(t('Updates for: @project', array('@project' => t('Drupal core'))), 'Translations found');
core/modules/locale/src/Tests/LocaleUpdateTest.php:    $this->assertText('Updates for: Contributed module one, Contributed module two, Custom module one, Locale test', 'Updates found');
core/modules/locale/templates/locale-translation-update-info.html.twig:    <span class="locale-translation-update__message">{% trans %}Updates for: {{ module_list }}{% endtrans %}</span>

The right source string to use in the test would be t('Updates for @module_list') NOT t('Updates for: @modules') or t('Updates for: @project'). The next one:

$ git grep "%title has been created"
core/modules/filter/src/Tests/FilterAdminTest.php:    $this->assertRaw(t('Basic page %title has been created.', array('%title' => $edit['title[0][value]'])), 'Filtered node created.');
core/modules/forum/src/Tests/ForumTest.php:      $this->assertNoRaw(t('@type %title has been created.', array('@type' => $type, '%title' => $title)), 'Forum topic was not created');
core/modules/forum/src/Tests/ForumTest.php:      $this->assertRaw(t('@type %title has been created.', array('@type' => $type, '%title' => $title)), 'Forum topic was created');
core/modules/node/src/NodeForm.php:      drupal_set_message(t('@type %title has been created.', $t_args));
core/modules/node/src/Tests/NodeCreationTest.php:    $this->assertRaw(t('@post %title has been created.', array('@post' => 'Basic page', '%title' => $edit['title[0][value]'])), 'Basic page created.');
core/modules/node/src/Tests/NodeCreationTest.php:    $this->assertRaw(t('@post %title has been created.', array('@post' => 'Basic page', '%title' => $edit['title[0][value]'])));
core/modules/system/src/Tests/Entity/EntityTranslationFormTest.php:    $this->assertRaw(t('Basic page %title has been created.', array('%title' => $edit['title[0][value]'])), 'Basic page created.');
core/modules/taxonomy/src/Tests/TermTest.php:    $this->assertRaw(t('@type %title has been created.', array('@type' => t('Article'), '%title' => $edit['title[0][value]'])), 'The node was created successfully.');

Where it tests t('@post %title has been created.') or t('Basic page %title has been created.') all of those are wrong and should be t('@type %title has been created.') based on the similar string that is actually output.

The only reason the wrong assertions pass is we don't run the tests translated :) Given the source strings are different, they would not pass with translations that may be different.

These need to be resolved in core, these are testing bugs.

gábor hojtsy’s picture

For the RSS examples, they indeed come from test modules, I would argue the test module should not t() the source either, unless it needs to test the translation. For the last one, that is as your snippet shows too, comes from Drupal's node type list builder, so not test only either:

$ git grep "No content types available."
core/modules/node/src/NodeTypeListBuilder.php:    $build['table']['#empty'] = $this->t('No content types available. <a href=":link">Add content type</a>.', [
core/modules/node/src/Tests/NodeTypeTest.php:    $this->assertRaw(t('No content types available. <a href=":link">Add content type</a>.', [
droplet’s picture

Priority: Major » Minor
Status: Needs work » Closed (won't fix)

OK. Get some more info. It's much less than my assumption. After deduplication, there're less than 225 strings. At this level, it's more suitable to do something in Coder-like modules instead.

(So in other hand, we may have 225 trivial bugs / untidy code in CORE)

Thanks ALL.

gábor hojtsy’s picture

@droplet: it would be a great meta issue to crowd-source :)

gábor hojtsy’s picture

Project: Localization server » Translation template extractor
Status: Closed (won't fix) » Needs review
StatusFileSize
new574 bytes

So I made up my mind because I heard this from several more people. Also apparently we already try to avoid parsing tests. We exclude the ..../tests/... directories, so test modules are not included. See https://localize.drupal.org/translate/projects/drupal/releases/454190 there are no such files parsed. However, there are 1550 files that are ..../Tests/.... (note case difference). Those are the actual tests. We should exclude those too probably. Not sure of the overall impact of them, but I made up my mind about making people work on those. Why would they. You are right. I was wrong.

Status: Needs review » Needs work

The last submitted patch, 13: 2600816.patch, failed testing.

gábor hojtsy’s picture

Title: Filtering Test Cases Strings » Strings from test cases are parsed, they should not be
Version: 7.x-1.x-dev » 7.x-3.x-dev
Category: Task » Bug report
Priority: Minor » Major
Status: Needs work » Needs review
Issue tags: +Drupal 8 compatibility

Gábor Hojtsy queued 13: 2600816.patch for re-testing.

  • Gábor Hojtsy committed c2608f1 on 7.x-3.x
    Issue #2600816 by Gábor Hojtsy, droplet: Strings from test cases are...
gábor hojtsy’s picture

Status: Needs review » Fixed

Ok committed. Now deploying to drupal.org.

gábor hojtsy’s picture

Ok this got rid of 496 strings. Not a small feat to avoid people working on things they will not need. Ideally tests would only ever use strings that exist on the frontend only, but its clear that tests also use test modules and strings from there, as well as several bogus strings.

droplet’s picture

Great! Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.