Closed (fixed)
Project:
Drupal core
Version:
8.4.x-dev
Component:
postgresql db driver
Priority:
Critical
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
8 Nov 2017 at 14:17 UTC
Updated:
28 Nov 2017 at 13:09 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
alexpottSo reverting HEAD back to 113c6dc258 / #2840392: Enable BigPipe by default in the Standard install profile because the test run was passing 2 days ago.
This reverts:
Comment #3
tacituseu commented@alexpott: see #2871591-167: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts, also @vaplas made it pass with second patch from #2879048-183: Ignore: patch testing issue for #2919863.
Also failing on SQLite (https://www.drupal.org/pift-ci-job/803921).
Comment #4
mradcliffeHmm, the test runs seem to be failing all over the place with #2: https://dispatcher.drupalci.org/job/drupal_patches/36937/consoleFull, https://dispatcher.drupalci.org/job/drupal_patches/36936/consoleFull, https://dispatcher.drupalci.org/job/drupal_patches/36938/consoleFull, https://dispatcher.drupalci.org/job/drupal_patches/36939/consoleFull
Comment #6
alexpottI borked the patch - here's a new one... by including my customised .htaccess.
Comment #7
mradcliffeShoot. It looks like 5.5 test run has the same fail on #6: https://dispatcher.drupalci.org/job/drupal_patches/36959/consoleFull
Comment #8
Anonymous (not verified) commentedAs @tacituseu said in #3, we already have few issues about this problems, that can be useful here.
Comment #9
alexpottIt's weird that #6 failed whilst https://dispatcher.drupalci.org/job/php5.5_postgres9.1/2567/consoleFull was a pass. That is the log of the last successful branch test on php5.5 / postgres / 8.5.x and the commit hash was:
So the codebase is exactly the same. This suggests that something else has changed. Perhaps on DrupalCI or on the underlying AWS infrastructure.
Comment #10
tacituseu commentedRe #9: the codebase isn't the same, your revert is missing #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts.
Comment #11
alexpott@tacituseu interesting - according to my git log #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts was committed before 113c6dc
Comment #12
tacituseu commented@alexpott: my bad, I somehow assumed you ment to revert all of Nov 6.
The fail isn't 100%, e.g. for SQLite:
https://www.drupal.org/pift-ci-job/805468 is green
while
https://www.drupal.org/pift-ci-job/803921
https://www.drupal.org/pift-ci-job/804598
had the same problem
Comment #13
alexpott@tacituseu well the fail seems to be 100% for PHP5.5 + Postgres + 8.5.x at the moment and it wasn't until the commit after 113c6dc. But the fact that revert all the changes doesn't fix it does seem to suggest that something else is at play other than the 8.5.x codebase.
Comment #14
alexpott@vaplas so any ideas why enabling book seems to the issue?
Comment #15
tacituseu commentedLooking at @vaplas patches it fails 2-8 times per 30 runs of affected tests, both SQLite and PostgeSQL.
Edit:
@alexpott: Retested #6 on PostgreSQL and it passed (https://www.drupal.org/pift-ci-job/806292).
Comment #16
Anonymous (not verified) commented#14: This patch from here. It is change the set of dependencies, the collection of which leads to random bugs in PHP 5.x. Maybe this bug (found by @xjm, see more in #2919863: Discover why gc_disable() during non-interactive install improves tests stability).
But why for Postgresql and SQLite need different tests? And why it does appears on PHP 5.6 as before? I do not know. See also #2879048-189: Ignore: patch testing issue for #2919863 from @tacituseu.
#15: No, just different modules are differently useful. And 'tour' is useless even here) Three other modules mixed the cards well enough)
Comment #17
tacituseu commented@vaplas: #15 was meant to be a reply to #13.
Comment #18
alexpottI wonder if we shouldn't just disable garbage collection during non-interactive Drupal install during tests.
Comment #19
wim leers#18 looks sensible.
Comment #20
tacituseu commentedSure beats reverting important patches each time we step on unsupported PHP's toes, all just because of some vintage Ubuntu fetish ;).
Comment #21
Anonymous (not verified) commentedFunny, the opposite approach in #2879048-202: Ignore: patch testing issue for #2919863 also works!)
Comment #22
catchThe revert patch in the other issue is likely because it's affecting how the tests interact at high concurrency, however it's very likely to result in re-introducing previous segfaults we've already fixed. Moving this to RTBC.
Comment #23
Anonymous (not verified) commented+1. Perhaps large test can cause this bug even with the
gc_disable()during the installation phase. But this patch is in any case a good strengthening before #2842431: [policy] Remove PHP 5.5, 5.6 support in Drupal 8.7Comment #24
tacituseu commented@catch: that revert wasn't meant to be a solution, just a check to see if it will move the segfault out of
EntityTest*JsonInternalPropertyNormalizerTest, it shows that there's nothing inherently wrong with #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts itself.+1.
Comment #25
wim leers#22: which revert patch? One of the patches in #2879048: Ignore: patch testing issue for #2919863? Reverting #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts or #2871591: Allow ComplexData in TypedData to specify computed properties that should be exposed in normalization and other contexts would be a massive blow to the API-First Initiative.
Comment #26
Anonymous (not verified) commented#25: no) @catch said about #21, where I pointed to @tacituseu's patch with demonstration of passing tests after revert #2859704: Intermittent segfaults on DrupalCI (some "did not complete due to a fatal error" with no additional info).
In #24 @tacituseu already confirmed that he did not pursue the goal of pushing this revert. I'm sorry if my #21 comment has caused confusion.
Comment #27
catchYep sorry that wasn't really for tacituseu just for anyone looking at this issue in 4 years who might get confused, but now everyone's confused now...
Committed/pushed to 8.5.x and cherry-picked to 8.4.x. Thanks!
Comment #30
Anonymous (not verified) commented🎉
Hah!) It means that in four years anyone looking at this issue will feel more comfortable with confused :)
I reruned #2879048-178: Ignore: patch testing issue for #2919863 with test-only for PostgreSQL and SQLite, and now both green as expected. Super!
#2919863: Discover why gc_disable() during non-interactive install improves tests stability fixed as support request, because now we know what happened. Thank you all, and especially @tacituseu, who did a lot of work on this, and many times corrected me in my research. 🙏
Comment #31
Anonymous (not verified) commentedBy the way, @Lendude also made a significant contribution (#2879048-98 - #2879048-109), creating a very useful test at an early stage research #2879048-102: Ignore: patch testing issue for #2919863.
Comment #32
wim leers🎉👏
Comment #33
tacituseu commentedThis would also mean #2900399: Adding test coverage for batch bulk updates can go in now.
Comment #35
catch