| [11:26:38] | <mstenta[m]> | Hmm something is causing farmOS tests to fail. The last scheduled run (7 hours ago) failed: https://github.com/farmOS/farmOS/actions/runs/10055315529/job/27792170166 |
| [11:27:02] | <mstenta[m]> | And a PR that was previously passing is failing in the same way, so it still seems to be happening (this is how I noticed it). |
| [11:27:18] | <mstenta[m]> | https://github.com/farmOS/farmOS/actions/runs/10061424801 |
| [11:28:01] | <mstenta[m]> | The second to last scheduled run (yesterday morning) passed, and nothing changed in our code between the two: https://github.com/farmOS/farmOS/actions/runs/10037210520 |
| [11:28:46] | <symbioquine[m]> | Any dependencies change? |
| [11:29:13] | <mstenta[m]> | I've seen this failure message before (Fatal error: Uncaught AssertionError: assert(count($nodes) === 1) in /opt/drupal/vendor/brianium/paratest/src/Logging/JUnit/TestCase.php:79), and I recall it being very unhelpful and difficult to diagnose, but I can't remember what caused it in the past. 🤔 |
| [11:29:33] | <mstenta[m]> | Something upstream must have changed yea |
| [11:30:30] | <symbioquine[m]> | Arguably, this is why we should be checking in composer.lock right? |
| [11:31:09] | <mstenta[m]> | That might prevent this, yes. But I think there are other reasons not to check in composer.lock. |
| [11:32:05] | <mstenta[m]> | And (arguably), not checking in composer.lock serves to alert us like this. So we know it's upstream and not related to an actual change. Although in this case the error message isn't very helpful. |
| [11:33:14] | <symbioquine[m]> | mstenta[m]: Or better IMHO, it breaks in a PR where the only thing that was changed was upgrading our `composer.lock` |
| [11:34:10] | <symbioquine[m]> | Obviously, it has the downside that such PRs have to occur regularly or else the resulting churn/breakage is much harder to untangle |
| [11:35:08] | <mstenta[m]> | Related to the composer.lock idea... I proposed committing that in this PR: https://github.com/farmOS/farmOS/pull/739, but it wouldn't actually be committed in farmOS/farmOS, it would be in farmOS/composer-project... so not exactly relevant to this issue |
| [11:36:13] | <paul121[m]> | Just came here to report this 😄 - it's breaking my rothamsted tests that use the same farmOS dev image. |
| [11:36:16] | <mstenta[m]> | Regardless of where/how the error gets thrown... I wonder what's causing this one... 🤔😅 |
| [11:37:02] | <paul121[m]> | Although the error is slightly different: https://github.com/Rothamsted-Ecoinformatics/farm_rothamsted/actions/run... |
| [11:37:43] | <mstenta[m]> | Hmm |
| [11:38:01] | <paul121[m]> | In my tests' setup method the`$this->drupalLogin($user)` is raising an error: |
| [11:38:01] | <paul121[m]> | > 1) Drupal\Tests\farm_rothamsted_experiment_research\Functional\ResearchAccessTest::testProposalAccess |
| [11:38:01] | <paul121[m]> | Behat\Mink\Exception\ElementNotFoundException: Button with id|name|label|value "Log in" not found. |
| [11:38:30] | <paul121[m]> | Maybe the assert(count($nodes) === 1 error from core is related to some missing "log in" button. |
| [11:39:40] | <mstenta[m]> | A missing button could indicate that something bigger is going wrong in the render pipeline generally |
| [11:39:50] | <mstenta[m]> | eg: maybe it's a 500 error / white screen of death |
| [11:40:08] | <paul121[m]> | True |
| [11:40:08] | <mstenta[m]> | (although I think that would show in the logs) |
| [11:41:39] | <mstenta[m]> | paul121: I assume your tests are not using `paratest`? |
| [11:41:54] | <mstenta[m]> | Maybe disabling paratest in the core workflow temporarily would reveal something |
| [11:42:20] | <paul121[m]> | Correct |
| [11:42:22] | <mstenta[m]> | eg: maybe paratest doesn't bubble up certain types of errors that break low level stuff |
| [11:42:53] | <mstenta[m]> | (again, I recall this error from a past issue... I forget how I debugged it, but that may be what I did) |
| [11:43:29] | <symbioquine[m]> | mstenta[m]: Sounds like a good idea. Should only make tests run slower, can be re-enabled once issue is resolved. |
| [11:44:23] | <mstenta[m]> | Testing now: https://github.com/mstenta/farmOS/actions/runs/10062068937 |
| [11:45:34] | <mstenta[m]> | While we're waiting for that... anyone want to review https://github.com/farmOS/farmOS/pull/855? :-) |
| [11:46:05] | <symbioquine[m]> | Hmmm, that one. I've been meaning to look at it... |
| [11:46:59] | <mstenta[m]> | That PR is the simplified (and first step) version of https://github.com/farmOS/farmOS/pull/751 (which was a bit harder to review because of overlapping changes) |
| [11:47:41] | <mstenta[m]> | I think we #855 is "complete" in the sense that it can be merged standalone without needing to wait for the other changes proposed in #751 |
| [11:52:20] | <symbioquine[m]> | Just to make sure I'm understanding, the change from hard-coding /opt/drupal to using the DRUPAL_PATH variable isn't critical to that change right - just a separate DRY improvement? |
| [11:52:29] | <mstenta[m]> | Correct |
| [11:52:50] | <mstenta[m]> | iirc only the last commit is actually the change as described by the PR name |
| [11:53:02] | <mstenta[m]> | the commits before that are prep |
| [11:53:58] | <mstenta[m]> | and general cleanup that was easy to agree on :-) |
| [11:57:14] | <paul121[m]> | I think this looks good to me. I hope we can get the other changes in soon, they do a lot for simplifying all of this docker stuff IMO |
| [11:57:42] | <mstenta[m]> | Agreed |
| [11:57:52] | <paul121[m]> | Specifically the install steps, there is a lot that can be simplified |
| [11:58:38] | <mstenta[m]> | You mean composer install? |
| [11:59:23] | <mstenta[m]> | Yea, this PR opens the path to disolving build-farmOS.sh entirely... which only existed because we had two separate Dockerfiles that needed to do the same thing |
| [11:59:54] | <mstenta[m]> | And it should speed things up a bit, since ideally we won't need to run composer install TWICE for dev image |
| [12:00:04] | <mstenta[m]> | (once to build prod, and again to replace it with dev) |
| [12:01:10] | <paul121[m]> | Yeah. It's all connected really, but whatever is easier for us to move forward with. My head is in it now so happy to make a PR for some of the next steps |
| [12:02:43] | <symbioquine[m]> | paul121[m]: Might make sense to coordinate with @AlMaVizca for any bits that overlap with the originally proposed changes so they get credit for those changes... |
| [12:02:43] | <mstenta[m]> | That would be great paul121 ! I only had enough time/brain space to get as far as #855 myself |
| [12:03:04] | <mstenta[m]> | Yea I tried to maintain their commit credits as much as possible in #855 |
| [12:03:23] | <mstenta[m]> | (it was harder with the big overlapping changes though) |
| [12:05:25] | <mstenta[m]> | Thanks for the approvals! |
| [12:06:36] | <mstenta[m]> | Hmm interesting... disabling paratest did provide some new info... https://github.com/mstenta/farmOS/actions/runs/10062068937/job/27814074417 |
| [12:06:51] | <mstenta[m]> | Lots of errors... |
| [12:06:53] | <mstenta[m]> | Many Button with id|name|label|value "Log in" not found. |
| [12:07:25] | <mstenta[m]> | Ah there is a 500 error in one... |
| [12:07:31] | <mstenta[m]> | 1) Drupal\Tests\farm_api\Functional\CorsResponseEventSubscriberTest::testCorsResponseHeaders |
| [12:07:31] | <mstenta[m]> | Failed asserting that 500 is identical to 200. |
| [12:26:32] | <paul121[m]> | Weird |
| [12:37:02] | <mstenta[m]> | I haven't had a chance to dig much deeper... and I need to go run some errands right now. If you discover anything let me know! |
| [15:51:14] | <paul121[m]> | Just taking another look... I went to one of my production farmOS sites and looked to see what modules have available updates. drupal/consumers had a release 7/22 and 7/23, the latter fixing this issue: https://www.drupal.org/project/consumers/issues/3463248 |
| [15:52:28] | <paul121[m]> | Re-running core farmOS tests and think they might pass now 🤞 |
| [16:00:29] | <paul121[m]> | No luck with the latest drupal/consumers 1.19... I wonder if we go back to 1.17 if that will work. |
| [16:26:45] | <paul121[m]> | That seemed to work. So maybe drupal/consumers 1.18 and 1.19 are both broken. I can't seem to replicate this locally though. |
| [17:15:05] | <paul121[m]> | I could replicate the issue with 1.18 once I deleted all consumer entities. 1.19 seems to be working locally, but still is failing in automated tests. Not exactly sure why |