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.| Comment | File | Size | Author |
|---|---|---|---|
| #31 | 2031175-import-disable-31.patch | 4.55 KB | vijaycs85 |
| #31 | 2031175-diff-30-31.txt | 654 bytes | vijaycs85 |
| #30 | 2031175-import-disable-30-interdiff.txt | 878 bytes | Berdir |
| #30 | 2031175-import-disable-30.patch | 4.55 KB | Berdir |
| #27 | interdiff-2031175-24-26.txt | 2.5 KB | Sutharsan |
Comments
Comment #1
Gábor HojtsyYes! Should make run tests much faster hopefully.
Comment #2
penyaskitoSomething like this?
Comment #3
Sutharsan CreditAttribution: Sutharsan commentedI expected this patch to fail. If disabled by default, the translation import/update test require this setting to be enabled explicitly. Since this test is passed, I doubt that it effectively disables the the translation download.
Comment #4
Gábor HojtsyUnit tests would not even enable modules, right? Should be in webtestbase AFAIS.
Comment #5
BerdirI think the best place for this would be in the testing profile config folder (overriding the config provided by locale... might not yet but should work) or testing_install().
Comment #6
jair CreditAttribution: jair commentedNeeds reroll
Comment #7
BerdirChanging this to a bug, might even be major.
Tests that enable languages through the UI are considerably faster like this. And we have tons of those, getting 44 matches for "$this->drupalPost('admin/config/regional/language/add'".
Locally, this gets the TwigTransTest from 3m22 to ~1min. Probably not such a big difference on the bot, hard to tell.
Copied the locale settings directory into the testing profile. Could also live in testing_install() but that wouldn't be as fancy ;) Downside of this is that we have to duplicate the whole file.
Comment #8
Gábor HojtsyTests could also use global overrides from $conf?
Comment #9
BerdirNot really, we would have to write that into a test settings file so that it can be loaded on page requests. We do support and use that for some settings() tests, but doing it all the time when we can just change the configuration is IMHO backwards :) I'd rather add it to testing_install().
Comment #10
Gábor HojtsyOk, maybe do that then? I think keeping a full config file like in #7 in sync sounds like can be problematic.
Comment #11.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #12
penyaskitoDisabled import_enabled in hook_install in testing profile.
Should fail for the translation update tests.
Comment #13
BerdirShould use \Drupal::config().
Somehow didn't work, because the tests didn't fail.
Comment #14
penyaskitoAs locale installation overrides config in the profile/profile hook_install(), we must do this on hook_modules_installed().
Comment #15
penyaskito@Sutharsan: I reviewed the translate tests and AFAICS all of them manipulate the status data, so the variable doesn't really affect them. Probably we are missing test coverage. Please correct me if I am wrong.
This looks really promising. #14 took 39 min 6 sec while #12 took 1 hour 25 min.
Comment #16
penyaskito7: import-enabled-testing-2031175-7.patch queued for re-testing.
Comment #19
vijaycs85Updating patch with config save().
Comment #21
BerdirHah, yeah, saving helps :) Might be worth to check if the first, easier approach would work too?
I think some changes are being discussed as to who wins in such a scenario, might be worth to check with @alexpott.
Comment #22
Sutharsan CreditAttribution: Sutharsan commentedRe-enabling the translation import during locale tests.
Comment #23
Sutharsan CreditAttribution: Sutharsan commentedThis one is using Berdir's approach in #7 to disable the import in the profile settings.yml file.[edit] Ignore this code. Bad patches.
Comment #25
Sutharsan CreditAttribution: Sutharsan commentedOeps, wrong patches.
This one is using Berdir's approach in #7 to disable the import in the profile settings.yml file.
@penyaskito Re. #15:
translation.import_enabledonly disables import when modules or themes are enabled and when languages are added. Some LocaleUpdate tests do cover these processes, others don't. The ones involved do fail (on my localhost) with #22 and #23 type solutions. But I did not provide a failing patch (yet).Comment #27
Sutharsan CreditAttribution: Sutharsan commentedThe failing tests are caused by configuration translations which are affected by the
translation.import_enabledconfiguration too. Not use if that was the desired effect, but we can open an new issue for that.This patch re-enables the import for these tests.
Comment #30
BerdirFixing the last test fail, only a single method there is failing.
Did some tests, what I could see is that the difference is ~20s per language that is added. Obviously, it only is like that if locale.module is enabled. Haven't counted how many times 20s we're talking about :)
Comment #31
vijaycs85Reviewed this patch. Overall it looks great. Minor update on config file... +1 to RTBC.
Comment #32
Gábor HojtsyLooks good indeed.
Comment #33
xjm31: 2031175-import-disable-31.patch queued for re-testing.
Comment #34
webchickGreat catch!
The only concern I have here is this is an awful lot of code +docs to copy/paste/get identical each time this needs to be done. I wonder if we could open up a follow-up to get all of these tests deriving from a base class w/ a helper method on it, or... something like that.
Anyway, this works on its own and makes tests faster, so yay!
Committed and pushed to 8.x.
Comment #35
Gábor HojtsyThis should help with Drupal 8 velocity in general by allowing faster test feedback :) Thanks all!
Comment #36
BerdirThe biggest improvement is running those tests yourself, doesn't matter that much if we save 10 or 20 times 20 seconds, distributed across 8 parallel processes, but having to wait 20s less on a test is nice :)