Converged naming, failure messages, test structure and comments onto the existing project conventions. - #925
Open
AlexSkrypnyk wants to merge 38 commits into
Open
AlexSkrypnyk wants to merge 38 commits into
AlexSkrypnyk wants to merge 38 commits into
Conversation
…n 'EntityFieldParser' and 'MappingTrait'.
…bels in American English.
…nticator a manager.
…enerator gives the same directory elsewhere.
…hdogTrait' and 'BehatCliTrait' after the items they hold.
…functions that had one.
…hsTrait' and 'EckTrait' alphabetically.
…s that tested them for truthiness.
…rotected method each exposes, like the other 20.
… the record also describes the toolbox classes.
…at used single ones, matching the rest of the codebase.
… directly after the tests they serve.
…odsTest' after their last test, like the other test classes.
… 263 that restated the code, regenerating 'STEPS.md' and 'HELPERS.md'.
…ing 'STEPS.md', 'HELPERS.md' and the 'README.md' index.
…' like the other data providers.
…TraitTest' like every other test class.
…ailure sites in 'docs.php'.
…steps)', matching its factory.
…rs: 'positional and named keys' and '. Got %s.'
…hen the page shows no message of the type.
…tentBlockLoadMultiple()' from '$type' to '$content_block_type'.
…registry and config schema reader collaborators after their types.
…, so the string '0' counts as a value.
… but it should not', like the other 41.
…lTrait' message, the 'MappingTrait' option and 7 comments.
…he 3 messages that used it, like the other 14.
…s that used single quotes, like the other 500.
…ith a period, and reworded 'isn't configured!' as 'is not configured.'
…th', leaving 'By' to the lookups.
…' instead of 'should include', like the other step traits.
…r their event, as '<prefix><Event>()'.
…uthor()', as the unknown author message now quotes it.
…agListenerTest' data set that rejects a hook name in a skip tag.
…n_incomplete', the name the option, the messages and the locals use.
|
Important Review skippedToo many files! This PR contains 311 files, which is 11 over the limit of 300. To get a review, reduce the PR to 300 files or fewer by splitting it into smaller PRs or changing its base branch. Usage-priced reviews support at most 300 files. ⚙️ Run configuration
⛔ Files ignored due to path filters (3)
📒 Files selected for processing (311)
You can disable this status message by setting the
Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.x #925 +/- ##
=======================================
Coverage 95.96% 95.96%
=======================================
Files 164 164
Lines 8402 8402
=======================================
Hits 8063 8063
Misses 339 339 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
AlexSkrypnyk
added
the
Needs review
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Failure messages now read 1 way across the step traits and backends:
, but it should not, interpolated values in double quotes, a closing period andThe ... does not exist.. Alongside that,messageAssertExistsOfType()throwsElementNotFoundExceptionwhen the page shows no message of the type, 12 typed-string checks compare with=== ''so the string0counts as a value, and the meta robots steps readshould containandshould not contain.The same concept was spelled 2 or 3 ways depending on the trait it lived in. 10 hooks were named for their action while the rest were named for their event, 8 assertion and action methods qualified their target with
Bywhile the rest usedWith, collaborator properties such as$readerand$scenarioTagsdidn't match theirConfigSchemaReaderandScenarioTagRegistrytypes, and 693 source comments either restated the code or read as prose rather than technical statements. Reading 2 traits side by side meant meeting 2 vocabularies for 1 idea.Every renamed public method, parameter and hook is listed in
MIGRATION.md,STEPS.mdandHELPERS.mdare regenerated, and no step text changes apart from the meta robots pair. Public names whose convergence needs a maintainer's call, such as the log in spelling, the capability create and delete contracts and theBehat\Managernamespace, stay as they are and are tracked in #907 to #924.Before / After
Changes
Each inconsistency class is its own commit, so the branch reviews class by class.
Naming
setHookDispatcher(),getBrowserCapabilityResolver(),$scenarioTagRegistry,$configSchemaReader), withdocs/http-clients.mdand the class-browser diagram updated.<prefix><Event>():timeAfterScenario(),watchdogBeforeScenario(),bigPipeBeforeStep(),staticCacheAfterScenario(),entityLifecycleBeforeNodeCreate(),entityLifecycleAfterScenario()and the 4accessibility*()hooks. The architecture docs and diagrams follow.BytoWith(for exampleuserAssertExistsWithMail()andelementClickWithIndex()), leavingByto theFind,GetandExistslookups.contentBlockCreateSingle()andcontentBlockLoadMultiple()to$content_block_type.failOnIncompleteaccessibility aggregate key tofail_on_incomplete, the name the option, the messages and the locals already use.$matches,$context,$class_info, named loop variables,call<Method>()test bridges, American spelling, and test names that still called a registry or an authenticator a manager.Failure messages
, but it should notin the 18 messages that said, but should not.is not configured.instead ofisn't configured!.The ... does not exist.for the 4 outliers,get_debug_type()instead ofgettype()in 3, and theNameHandlermessages worded like the other field handlers.API shape
messageAssertExistsOfType()throwsElementNotFoundExceptionwhen the page shows no message of the type, which is what the project's exception table prescribes for a missing element.=== ''instead ofempty()at 12 sites, so the string0counts as a value.should containandshould not containinstead ofshould include, like every other step trait.docs.phpthrows\RuntimeExceptioninstead of a plain\Exception,TraitOptionResolvertakes(config, steps)like its factory, andpreg_match()results are compared with=== 1.Structure
FileDownloadTraitTest::dataProviderIsRegex()carry labels.EntityLifecycleTraitTestcallsparent::setUp()andparent::tearDown(), and the 2 file reuse kernel tests compare integer ids withassertSame().usestatements,fn(without a space, and doubled backslashes in the 30 namespace literals that used single ones.Comments
STEPS.md,HELPERS.mdand theREADME.mdindex are regenerated.Left for a decision
These need a maintainer's call, so they're filed instead of changed here:
Behat\Managernamespace).EmailTraitsubject loop), Prefix the '@trait:' and '@skipped' harness tags with 'test-' #916 (the@trait:and@skippedharness tags), Settle the remaining code-style ties in the library code #917 (code-style ties), Settle the split conventions in the test suite #918 (test-suite conventions).tableTransposeVertical()on an empty table), Reconcile the per-path page cache step with the cache tags it invalidates #920 (the per-path page cache step), Decide whether the '@queue' filter on the queue teardown is an activation tag #921 (the@queueteardown filter), Reconcile 'SupportedImageHandler' with how the file and image handlers reuse existing files #922 (SupportedImageHandlerand existing files), Reconcile the 'behat.dist.php' context registration with the one-context guard #923 (behat.dist.phpregistering both contexts), Decide whether the nested Behat run still needs the 'DRUPAL_FINDER_*' variables #924 (theDRUPAL_FINDER_*variables).2 small edits are left for a hand change:
behat.dist.phpstill registers bothWebContextandDrupalContext(#923), and CLAUDE.md still says the initializer "injects the user manager" (#915).Verification
ahoy test-bdd: 1,203 scenarios and 5,073 steps pass.accessibility.featurewas re-run on the last commit, which renames the aggregate key (11 scenarios pass).ahoy test-kernel: 125 tests. 1 assertion still expected the old single-quoted value in the unknown author message; it's fixed here and the class re-runs green.ahoy test-unit: 4,096 tests pass.ahoy lint: phpcs, phpstan, Rector, gherkinlint, the layer check and the trait check all pass.ahoy lint-docs: the generated docs are current.