Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add $defaultTheme property if missing on BrowserTestBase #211

Merged
merged 15 commits into from
Nov 21, 2023

Conversation

mglaman
Copy link
Collaborator

@mglaman mglaman commented Aug 10, 2022

Description

Add protected $defaultTheme = 'stark'; to functional tests if missing.

This could cause test failures because they assume on markup for Classy, which
the theme should be changed to starterkit as Classy is going away in Drupal 10.

To Test

Drupal.org issue

https://www.drupal.org/project/rector/issues/3299835

@mglaman
Copy link
Collaborator Author

mglaman commented Aug 10, 2022

It's running, but not making changes:

2022-08-10T16:07:45.9590491Z [file] web/modules/custom/rector_examples/test/src/Functional/BrowserTestBaseGetMock.php
2022-08-10T16:07:45.9590707Z [rule] DrupalRector\Rector\Class_\FunctionalTestDefaultThemePropertyRector

And here:

2022-08-10T16:07:44.9331621Z [file] web/modules/custom/rector_examples/tests/src/Functional/PublicStaticModulesTest.php
2022-08-10T16:07:44.9332120Z [file] web/modules/custom/rector_examples/tests/src/Functional/AssertFieldByIdTest.php
2022-08-10T16:07:44.9332384Z [rule] DrupalRector\Rector\Class_\FunctionalTestDefaultThemePropertyRector
2022-08-10T16:07:44.9332594Z [rule] DrupalRector\Rector\Deprecation\AssertElementPresentRector

@agentrickard
Copy link
Contributor

@mglaman This one has a merge conflict I am uncertain about

# Conflicts:
#	config/drupal-8/drupal-8.8-deprecations.php
…stuff to get working. Also fixed tests and how they run. Fixed codestyle
@bbrala bbrala force-pushed the functional-tests-default-theme branch from 0acf398 to 0f70eb0 Compare November 21, 2023 09:08
@bbrala
Copy link
Collaborator

bbrala commented Nov 21, 2023

This was one of the top issues last bot run. So thought I'd update and fix it. All seems fine now.

Copy link
Collaborator Author

@mglaman mglaman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve of the changes 👍 but it's my own PR so, this is my +1 for someone to approve and merge

Copy link
Collaborator

@bbrala bbrala left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per matt, approved

@bbrala bbrala merged commit 907a929 into palantirnet:main Nov 21, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants