- Issue created by @mstrelan
- @mstrelan opened merge request.
- Status changed to Needs review
over 1 year ago 3:34am 8 November 2023 - Status changed to Needs work
over 1 year ago 2:15pm 8 November 2023 - 🇺🇸United States smustgrave
Ran the script in the issue summary and got a different result
modified: core/modules/basic_auth/tests/src/Traits/BasicAuthTestTrait.php modified: core/modules/block/tests/src/Traits/BlockCreationTrait.php modified: core/modules/ckeditor5/tests/src/Traits/CKEditor5TestTrait.php modified: core/modules/ckeditor5/tests/src/Traits/PrivateMethodUnitTestTrait.php modified: core/modules/ckeditor5/tests/src/Traits/SynchronizeCsrfTokenSeedTrait.php modified: core/modules/config/tests/src/Traits/AssertConfigEntityImportTrait.php modified: core/modules/content_moderation/tests/src/Traits/ContentModerationTestTrait.php modified: core/modules/field/tests/src/Traits/EntityReferenceTestTrait.php modified: core/modules/field_ui/tests/src/Traits/FieldUiJSTestTrait.php modified: core/modules/field_ui/tests/src/Traits/FieldUiTestTrait.php modified: core/modules/jsonapi/tests/src/Traits/CommonCollectionFilterAccessTestPatternsTrait.php modified: core/modules/media/tests/src/Traits/MediaTypeCreationTrait.php modified: core/modules/media/tests/src/Traits/OEmbedTestTrait.php modified: core/modules/menu_ui/tests/src/Traits/MenuUiTrait.php modified: core/modules/migrate_drupal/tests/src/Traits/CreateMigrationsTrait.php modified: core/modules/migrate_drupal/tests/src/Traits/CreateTestContentEntitiesTrait.php modified: core/modules/migrate_drupal/tests/src/Traits/FieldDiscoveryTestTrait.php modified: core/modules/migrate_drupal/tests/src/Traits/NodeMigrateTypeTestTrait.php modified: core/modules/migrate_drupal/tests/src/Traits/ValidateMigrationStateTestTrait.php modified: core/modules/node/tests/src/Traits/ContentTypeCreationTrait.php modified: core/modules/node/tests/src/Traits/NodeCreationTrait.php modified: core/modules/system/tests/src/Traits/OffCanvasTestTrait.php modified: core/modules/system/tests/src/Traits/TestTrait.php modified: core/modules/taxonomy/tests/src/Traits/TaxonomyTestTrait.php modified: core/modules/user/tests/src/Traits/UserCreationTrait.php modified: core/modules/views/tests/src/Traits/ViewsLoggerTestTrait.php modified: core/tests/Drupal/Tests/ApiRequestTrait.php modified: core/tests/Drupal/Tests/BrowserHtmlDebugTrait.php modified: core/tests/Drupal/Tests/ConfigTestTrait.php modified: core/tests/Drupal/Tests/EntityViewTrait.php modified: core/tests/Drupal/Tests/ExtensionListTestTrait.php modified: core/tests/Drupal/Tests/PhpUnitCompatibilityTrait.php modified: core/tests/Drupal/Tests/RandomGeneratorTrait.php modified: core/tests/Drupal/Tests/RequirementsPageTrait.php modified: core/tests/Drupal/Tests/SchemaCheckTestTrait.php modified: core/tests/Drupal/Tests/SessionTestTrait.php modified: core/tests/Drupal/Tests/TestFileCreationTrait.php modified: core/tests/Drupal/Tests/TestRequirementsTrait.php modified: core/tests/Drupal/Tests/Traits/Core/Config/SchemaConfigListenerTestTrait.php modified: core/tests/Drupal/Tests/Traits/Core/CronRunTrait.php modified: core/tests/Drupal/Tests/Traits/Core/GeneratePermutationsTrait.php modified: core/tests/Drupal/Tests/Traits/Core/Image/ToolkitTestTrait.php modified: core/tests/Drupal/Tests/Traits/Core/PathAliasTestTrait.php modified: core/tests/Drupal/Tests/Traits/PhpUnitWarnings.php modified: core/tests/Drupal/Tests/UiHelperTrait.php modified: core/tests/Drupal/Tests/UpdatePathTestTrait.php modified: core/tests/Drupal/Tests/WaitTerminateTestTrait.php modified: core/tests/Drupal/Tests/XdebugRequestTrait.php
- Status changed to RTBC
over 1 year ago 2:20pm 8 November 2023 - 🇺🇸United States smustgrave
Actually think I misunderstood the ticket. As far as your comments think refactoring any trait/test could be it's own ticket.
- Status changed to Needs review
over 1 year ago 10:13pm 8 November 2023 - 🇦🇺Australia mstrelan
Just to clarify, pipeline 45766 demonstrates all the errors and fails when declare(strict_types=1) is added to all traits. The following commits then fix up the issues. Finally the initial commit is reverted so we have a reviewable MR. As discussed in #3397890-11: Fix strict type errors in unit tests → we decided that splitting the fixes and adding the declaration should be separated. We need a follow up issue here for adding the declaration (see the todo in the IS), which can just be a cherry-pick of the first commit.
- Status changed to RTBC
over 1 year ago 9:38am 9 November 2023 - Status changed to Needs work
over 1 year ago 2:10pm 12 November 2023 - 🇺🇸United States xjm
Under our test coding standards, it's preferred to use string literals when asserting test data. Neither
FormattableMarkup()
norsprintf()
should be used. (We've had massive cleanups previously to eliminate the usage of both in tests.) Thanks! - Status changed to Needs review
over 1 year ago 8:44pm 12 November 2023 - 🇦🇺Australia mstrelan
@xjm these are not in the actual assertions but in the $message argument.
- Status changed to Needs work
over 1 year ago 9:02pm 12 November 2023 - 🇺🇸United States xjm
@mstrelan, it goes for all arguments of assertions including the message. Actually even more so for the message; we stripped string formatting off that eight years back, whereas we did so only 3-5 years ago for asserted values.
What would be best would be to use the literal values rather than unnecessary uses of randomly generated test data, but that's out of scope.
Adding the related policy issue which apparently never got closed out and added to docs, but still applies.
- Status changed to Needs review
over 1 year ago 10:27pm 12 November 2023 - 🇦🇺Australia mstrelan
Not trying to argue here as I'm not bothered either way. Wondering what you mean "we stripped string formatting off that eight years back" since this and all the other related issues are removing usage of FormattableMarkup.
Regardless, I've converted these to string interpolation and it's back to NR.
- Status changed to RTBC
over 1 year ago 11:44pm 14 November 2023 - 🇺🇸United States xjm
Looks great now. So much cleaner. Committed to 11.x, 10.2.x, and 10.1.x as a test improvement. Thanks!
- Status changed to Fixed
over 1 year ago 9:01pm 15 November 2023 Automatically closed - issue fixed for 2 weeks with no activity.