Problem/Motivation

Start removing olivero from tests so Olivero can be deprecated

Steps to reproduce

Proposed resolution

Fix 'low hanging fruit' only. This is for tests where the install of olivero can be removed or changed to Stark without other test changes.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#6 3614964-nr-bot_981i4l3t.txt91 bytesneeds-review-queue-bot

Issue fork drupal-3614964

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

quietone created an issue. See original summary.

quietone opened merge request !16558

quietone’s picture

quietone commented
Issue summary: View changes
Status: Active » Needs review

The failing test is a CKEditor one that is not related.

quietone’s picture

quietone commented
Title: Remove install of oliver from tests » Remove install of Olilvero from tests
dcam’s picture

dcam commented

I left one question on the MR. Let me know what you think.

Also, I'm disturbed by how much repetition there is of arrays of theme machine names. Virtually none of these have newer themes. Do you think it's worth making a follow-up issue to create and implement a centralized list somewhere?

needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3614964-nr-bot_981i4l3t.txt91 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

quietone’s picture

quietone commented
Status: Needs work » Needs review

#5. I am inclined to think that it is not worth making a centralized list. There are only 20 files changed in this MR and not all of those have an array of themes that can be centralized. So, I'd rather we spend time on other things to fix.

dcam’s picture

dcam commented
Status: Needs review » Reviewed & tested by the community

Ok, that's fine.

My feedback has been addressed. Thank you for noticing that unused variable after the code block was removed. LGTM.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

One question on the MR.

mstrelan’s picture

mstrelan commented
Title: Remove install of Olilvero from tests » Remove install of Olivero from tests
quietone’s picture

quietone commented
Issue tags: +11.5.0 release priority
quietone’s picture

quietone commented
Status: Needs review » Reviewed & tested by the community

I made the small change requested by catch, which was to restore the use of olivero in UpdatePathTestBaseFilledTest.php. Since that is one line, there are no longer changes to that file in this MR, and tests are passing, I am restoring the RTBC.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs work

One question about SiteMaintenanceTest.

quietone’s picture

quietone commented
Status: Needs work » Needs review

The failing test is a known random one, core/modules/ckeditor5/tests/src/FunctionalJavascript/CKEditor5AllowedTagsTest.php, which I see had a fixed pushed recently.

catch’s picture

catch
he/him
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

This looks good to me now (well I also missed #13 so it looked good to me before that, but even better afterwards).

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Moving back to needs review for the open point in the MR discussion, we might just want a follow-up issue for that against default_admin?

acbramley made their first commit to this issue’s fork.

nitinkumar_7’s picture

nitinkumar_7 commented

tested MR localy !16558. Theme swaps/removals across the touched files are mechanical and in scope : no test logic changed beyond the theme reference. SiteMaintenanceTest drops one assertion. The Olivero-specific .messages__close AJAX assertion wasn't just deleted : it's relocated into a new core/themes/olivero/tests/.../AjaxTest.php, so that coverage isn't lost. No default_admin changes present.

nitinkumar_7’s picture

nitinkumar_7 commented

Tests pass locally for the affected files on my end. This looks correct and complete for the stated scope.
Looks good for me.

dcam’s picture

dcam commented

I can't review this anymore because I replaced Olivero with default_admin in that message-closing check. I left the justification for the change in an MR comment. It should address any concerns with the check being removed.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Reviewing @dcam commit https://git.drupalcode.org/project/drupal/-/merge_requests/16558/diffs?c... and change appears to make sense.

  • catch committed 29aa8874 on main 
    task: #3614964 Remove install of Olivero from tests

By: quietone
By:...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to main, thanks!

Will need a backport MR for 11.x

dcam opened merge request !16867

dcam changed the visibility of the branch 3614964-remove-olivero-from-tests-11 to hidden.

dcam changed the visibility of the branch 11.x to hidden.

dcam opened merge request !16868

acbramley’s picture

acbramley commented

Needed to fix the directory for the new test, for some reason that didn't fail on main? (FunctionalJavaScript vs FunctionalJavascript)

acbramley’s picture

acbramley commented

The test never ran on main either, created #3619402: Fix olivero AjaxTest directory to fix that.

dcam’s picture

dcam commented
Status: Patch (to be ported) » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems like a good backport

  • catch committed 80c6fecb on 11.x 
    task: #3614964 Remove install of Olivero from tests

By: quietone
By:...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.