Blocked on:

Fixed:

CommentFileSizeAuthor
#45 Add-drupalci.yml-3056710-45.patch1.17 KBsahana _n
#35 password_policy-8.x-3.x-ctools-and-deprecated-check-combined-3056710-35.patch888 bytesaohrvetpv
#34 3056710-ctools-and-deprecated-check-combined-8.patch1.02 KBjeroent
#33 3056710-ctools-and-deprecated-check-combined-7.patch1.02 KBjeroent
#26 3056710-ctools-and-deprecated-check-combined-6.patch1.17 KBjeroent
#25 3056710-ctools-and-deprecated-check-combined-5.patch1.04 KBjeroent
#21 3056710-ctools-and-deprecated-check-combined-4.patch5.08 KBjeroent
#20 3056710-ctools-and-deprecated-check-combined-3.patch5.08 KBjeroent
#19 3056710-ctools-and-deprecated-check-combined-2.patch5.06 KBjeroent
#18 3056710-ctools-and-deprecated-check-combined.patch5.05 KBjeroent
#12 interdiff-3056710-2-12.txt357 bytesaohrvetpv
#12 password_policy-8.x-3.x-add_drupalci_yml-3056710-12.patch556 bytesaohrvetpv
#6 3056710-6.patch8.91 KBjeroent
#5 3056710-5.patch7.99 KBjeroent
#4 3056710-3.patch7.33 KBjeroent
#3 3056710-2.patch6.94 KBjeroent
#2 add_drupalci_yml.patch635 bytesjeroent
add_drupalci.yml635 bytesjeroent

Comments

JeroenT created an issue. See original summary.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
StatusFileSize
new add_drupalci_yml.patch635 bytes
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
StatusFileSize
new 3056710-2.patch6.94 KB
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
StatusFileSize
new 3056710-3.patch7.33 KB
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
StatusFileSize
new 3056710-5.patch7.99 KB
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
StatusFileSize
new 3056710-6.patch8.91 KB
aohrvetpv’s picture

aohrvetpv commented

I like this--it'll save time and/or improve code quality. Will review for commit later.

I'm wondering if it's possible to also run the PHPCS DrupalPractice standard on commit? The warnings seem like they're just from the PHPCS Drupal standard.

aohrvetpv’s picture

aohrvetpv commented

How did you come up with the contents of this drupalci.yml? I found the following page with a sample drupalci.yml, but your drupalci.yml looks different:
https://www.drupal.org/drupalorg/docs/drupal-ci/customizing-drupalci-tes...

Trying to review but I'm not sure what a standard drupalci.yml looks like, or whether this one has too many or too few keys.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented

I found it at the examples module: https://git.drupalcode.org/project/examples/blob/8.x-1.x/drupalci.yml

aohrvetpv’s picture

aohrvetpv commented

Thanks. It looks like we may not need the container_composer: and nightwatchjs: lines, comparing the .yml in #2 versus the Examples module .yml.

I'm not sure what the sniff-all-files key does. (What is the default? What files would be sniffed if it were false?) Trying to find documentation...

aohrvetpv’s picture

aohrvetpv commented

I see now. Only changed files are sniffed by default. I think that's probably OK. (Why sniff all files if only changed ones can introduce phpcs failures?)

aohrvetpv’s picture

aohrvetpv commented
StatusFileSize
new password_policy-8.x-3.x-add_drupalci_yml-3056710-12.patch556 bytes
new interdiff-3056710-2-12.txt357 bytes

Status: Needs review » Needs work

The last submitted patch, 12: password_policy-8.x-3.x-add_drupalci_yml-3056710-12.patch, failed testing. View results

aohrvetpv’s picture

aohrvetpv commented
Status: Needs work » Needs review

We're not close to fixing all those exceptions that presumably caused the test failures. So maybe we should postpone adding the drupalci.yml? There's #3042612: Drupal 9 Deprecated Code Report to address the deprecations.

aohrvetpv’s picture

aohrvetpv commented
Status: Needs review » Needs work

Inadvertently changed status to needs review in prior comment.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Status: Needs work » Postponed
Issue tags: +DevDaysCluj

@AohRveTPV,

Allright. Will make separate issues for the remaining exceptions.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented

Created #3060837: Replace controller_resolver service with http_kernel.controller.argument_resolver in the ctools issue queue, which fill fix a lot of failing tests.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
StatusFileSize
new 3056710-ctools-and-deprecated-check-combined.patch5.05 KB

Combined the patch that added drupalci.yml and #3060837: Replace controller_resolver service with http_kernel.controller.argument_resolver and #3042612: Drupal 9 Deprecated Code Report to check if there are still other exceptions.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
StatusFileSize
new 3056710-ctools-and-deprecated-check-combined-2.patch5.06 KB
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
StatusFileSize
new 3056710-ctools-and-deprecated-check-combined-3.patch5.08 KB
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
StatusFileSize
new 3056710-ctools-and-deprecated-check-combined-4.patch5.08 KB
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Issue summary: View changes
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Issue summary: View changes
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
StatusFileSize
new 3056710-ctools-and-deprecated-check-combined-5.patch1.04 KB
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
StatusFileSize
new 3056710-ctools-and-deprecated-check-combined-6.patch1.17 KB
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Issue summary: View changes
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented

Wooow, the patch is finally passing all tests.

I currently created a drupalci.yml that will first apply the patches provided in the 4 issues, and afterwards will run all tests. This issue is still postponed and currently blocked on 2 ctools patches and 2 patches for the password_policy module.

aohrvetpv’s picture

aohrvetpv commented

Great work. I will review the patches and try to get them committed. I'm still a D8 novice so bear with me.

aohrvetpv’s picture

aohrvetpv commented
Status: Postponed » Active

I suppose once we get the Password Policy issues fixed, we could add a drupalci.yml that uses the container commands to apply the two Ctools patches.

aohrvetpv’s picture

aohrvetpv commented
Status: Active » Postponed
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented

@AohRveTPV,

allright. I'm creating a similar issue for ctools (#3060937: Add drupalci.yml) so we prevent incorrect test failures.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Issue summary: View changes
StatusFileSize
new 3056710-ctools-and-deprecated-check-combined-7.patch1.02 KB

Updated the patch to use the latest versions of the patches.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
StatusFileSize
new 3056710-ctools-and-deprecated-check-combined-8.patch1.02 KB
aohrvetpv’s picture

aohrvetpv commented
Status: Postponed » Needs review
StatusFileSize
new password_policy-8.x-3.x-ctools-and-deprecated-check-combined-3056710-35.patch888 bytes

Updated per fixing of #3060910: Add missing config_export definition to password_policy config entity type.

I think we could commit now, if this works, and update drupalci.yml later to remove the Ctools patching, if that seems like a good idea? Or should we wait on those Ctools issues?

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented
Issue summary: View changes
jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented

Discussed this at Drupal dev days and indeed, we can commit this now so we have extra quality checks and remove the patching later.

jeroent’s picture

jeroent
Primary language Dutch
Location 🇧🇪
commented

FWIW:


Jeroen Tubex [Today at 12:41 PM]
I’m trying to add a drupalci.yml file to the password_policy module: https://www.drupal.org/project/password_policy/issues/3056710
We currently still get errors when executing the deprecated code check because of ctools which is a dependency of the password_policy module. For now we added a custom container_command which applies patches to the ctools module so our tests succeed. But my question now is, should we commit it like this for now or should we wait until this is fixed in ctools? (edited)

3 replies

Jennifer Hodgdon  [31 minutes ago]
I recommend committing it for now, and following the ctools issues. As each ctools issue is closed, you can update your yml file... although if your tests are downloading a release of ctools (not the dev branch), you might have to wait for a new version of ctools before you update.


Jennifer Hodgdon  [31 minutes ago]
otherwise your tests will just continue to fail needlessly.


Jeroen Tubex  [1 minute ago]
Thanks for your input @jhodgdon (she/her)!

aohrvetpv’s picture

aohrvetpv commented
Status: Needs review » Fixed

Sounds good.

  • AohRveTPV committed c13db36 on 8.x-3.x 
    Revert "Issue #3056710 by JeroenT: Add drupalci.yml"

This reverts...
aohrvetpv’s picture

aohrvetpv commented
Status: Fixed » Needs work

Getting "Build Successful" lately instead of "X Passed" for the 8.x-3.x branch. I suspect this patch is causing it, so reverted. Will see if it says "X passed" now.

aohrvetpv’s picture

aohrvetpv commented

Reverting the commit fixed the problem. Not sure what's wrong with the drupalci.yml.

rosinegrean’s picture

rosinegrean commented
Issue tags: -DevDaysCluj +DevDaysTransylvania
sahana _n’s picture

sahana _n commented
Status: Needs work » Needs review
StatusFileSize
new Add-drupalci.yml-3056710-45.patch1.17 KB

Please review the patch.

Status: Needs review » Needs work

The last submitted patch, 45: Add-drupalci.yml-3056710-45.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented
Status: Needs work » Postponed (maintainer needs more info)

Thanks to everyone for the work on this issue.

I'm going through all the 8.x issues.

As the 8.x is no longer supported, I'm postponing this issue for now and need feedback as to whether or not this issue is relevant to 4.0.x.

If it is, please reopen and change the version, make sure the issue summary is clear and complete, including steps to reproduce, and reroll the patch. If it's not, please close.

If there is no response to this in a month addressing the above, it can be closed.