Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
This arose out of #52990: Improve translation string search and editing interface. We changed the path to the string translation page and all tests passed, despite the fact that we hadn't updated the path in 6 places.
I'm going to review the locale module's simple tests, and update / add as necessary, as part of the i18n sprint.
Cheers,
Stella
Comment | File | Size | Author |
---|---|---|---|
#14 | locale-369229-14.patch | 8.37 KB | nedjo |
#13 | locale-369229-13.patch | 8.34 KB | nedjo |
#12 | locale_369229.patch | 2.46 KB | stella |
#8 | locale_tests_369229.patch | 52.24 KB | stella |
#6 | locale_tests_369229.patch | 50.6 KB | stella |
Comments
Comment #1
stella CreditAttribution: stella commentedComment #2
stella CreditAttribution: stella commentedFixed some string concatenations.
Comment #4
stella CreditAttribution: stella commentedUpdated patch with additional test for automatic module translation file import when a language is enabled. I've also verified the patch applies.
I'd like to do a similar check for automatic module translation file import when a module is enabled, but unsure of the best way to do it. I need to create a language, and then place a po file in a disabled module's translations/ directory before enabling the module. Ideally I'd like to enable the module via admin/build/modules, but this means I can't use a "mock module" since they don't appear on the module list. Without making a change to how mock modules work, the only way I can create a test for it is by adding the translation file to a real core module's directory. Would this be acceptable to people? The file would be removed after the import.
Comment #6
stella CreditAttribution: stella commentedFingers crossed...
Comment #7
catchRecently webchick's asking for all inline comments to have a line break above, not sure how set in stone that is yet though.
extra line break. There's a couple of these.
Comment is already there in the test but it's a bit clunky, but how about this instead?
Should this be a @TODO?
Not sure if we do it for methods, but usually we add () after function names in comments to help them stand out.
Just 'import' maybe? Looks like both might be technically correct and it comes down to usage.
Not touched by the patch, but should be 'created' here.
Do these buttons do exactly the same thing? Could we make them "Export foo" and "Export bar"? ~(might be kitten killing in this issue though). Maybe add a @TODO for the submit button id?
Looks like it adds a lot of coverage, and does some really nice cleanup of the existing test as well. Nice! Feel guilty marking as CNW :(
Comment #8
stella CreditAttribution: stella commentedRe-rolled version of the patch with updates following on from catch's comments.
Just for
it's not that it's a TODO item, but rather the SimpleTest API doesn't seem to support the submit button ids - even though the docs at http://cwgordon.com/simpletest/doxygen/2008-june/index.html though maybe those docs are out of date.
Cheers,
Stella
Comment #9
Dries CreditAttribution: Dries commentedCommitted to CVS HEAD. Thanks!
Comment #10
mr.baileysD7 patch in #296584: Incorrect t()ing of menu item title in example fails at one of the new tests.
Strange thing is: the (single-line) patch only changes
menu.api.php
(documentation) and is trivial. I couldn't find more information about which specific assertion failed so I don't have a clue where to start looking, but I don't see a reason why the patch would fail.I was hoping someone involved in adding these tests can help me out here.
Comment #11
stella CreditAttribution: stella commentedI just re-ran the tests with a clean install of HEAD with your patch applied and all tests passed. You should probably request a re-test.
Restoring 'fixed' status. Please post all follow-ups to your issue at #296584
Cheers,
Stella
Comment #12
stella CreditAttribution: stella commentedRe-opening this issue as I want to make a change to one of the committed tests. The above patch creates a .po file in the modules/locale/tests/locale_tests/translations/ directory. However, not all drupal installations will have access to write to this directory, so I want to change the patch so this test po file is already present. The attached patch does this. All tests pass.
Cheers,
Stella
Comment #13
nedjoI hit this error: no write access to create the modules/locale/tests/translations directory.
So, indeed, rather than dynamically generating a translation file, we need to have one present for testing purposes.
To avoid unintended results - e.g., regular translations files being imported along with test ones - we need to set an invalid language code for some test files. This patch uses the invalid code xx. In some cases we will want a second language code (e.g., to test an enabled vs. a disabled language). We can use an extended code for this: xx-yy.
For consistency, it makes sense to use the same testing language codes in the various places where we need them, rather than retaining the previous randomly generated ones.
Tested locally. I'll wait for the testbot to confirm this before marking RTBC.
Comment #14
nedjoFix an error in the path to the added file.
Comment #15
webchickI got a failure before applying this patch. Now it works fine. Committed to HEAD. Thanks!
I'm going to try switching testing bot on again... wish me luck. ;P
Comment #16
stella CreditAttribution: stella commentedI'm not sure this patch addresses the issue in #381004: locale test is failing.... this patch addresses the issue where the user can't write to the modules dir, but that wasn't happening in the 'user language settings' set of tests.