Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
ConfigInstallProfileOverrideTest is using tour. Need to decouple that
Steps to reproduce
Proposed resolution
Create a custom test module with optional config and update/replace tour config in test_config_overrides profile with config similar to new test module. To mimic what tour was previously doing in ConfigInstallProfileOverrideTest.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Issue fork drupal-3387163
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:
changes
Comments
Comment #3
smustgrave CreditAttribution: smustgrave at Mobomo commentedEnded up reusing actions as they were already included.
Comment #4
xjmA test of the configuration system should not rely on any core feature. Coupling test coverage to anything unnecessary to the test is bad and causes no end of false fails, technical debt, and weird problems. We should therefore always use test fixtures instead of real core features unless the feature itself is what's being tested. If Tour can be swapped for Action without any impact on the coverage or intended functionality of the test, it can also be swapped for a test module with a similar configuration structure. Thanks!
Comment #5
smustgrave CreditAttribution: smustgrave at Mobomo commentedAll green
Comment #6
larowlanLeft a comment, we already have
\Drupal\config_test\Entity\ConfigTestso I think we can simplify this even furtherComment #7
smustgrave CreditAttribution: smustgrave at Mobomo commentedSwitched up to using entity_test_bundle.
Comment #8
larowlanLooks good to me, nice one!
Comment #9
alexpottNice decoupling from the tour module but unfortunately I think we've subtly changed the nature of what is being tested here and given how optional configuration is handled during installation I think the changes are important. I've added comments to the MR in gitlab.
Comment #10
smustgrave CreditAttribution: smustgrave at Mobomo commentedAddressed feedback
Comment #11
smustgrave CreditAttribution: smustgrave at Mobomo commentedUpdated
Comment #12
larowlanI think this is ready again, but I will admit I've struggled to get my head around it.
Looking at it line by line, it looks to me like the removed tour config has been replaced with equivalent test config entities
Comment #13
alexpottAdded some more comments to the MR.
Comment #14
smustgrave CreditAttribution: smustgrave at Mobomo commented@alexpott think I addressed the issues.
Comment #15
quietone CreditAttribution: quietone at PreviousNext commentedCame by here to move issues along for the tour module.
@smustgrave, thanks for working on this and learning about the configuration system on the way!
I agree with @larowlan! A completed issue summary is extremely valuable to anyone working on an issue and this one is empty. :-( What this, and every issue, should have is a high quality issue summary that explains what the code is doing with as much detail as needed so contributors don't need to start from scratch. Because of the time spent figuring out the changes, I did not review the code (and beside larowlan and alexpott already have) so I stuck to the comments. Setting to NW for improvement in that area.
Comment #16
smustgrave CreditAttribution: smustgrave at Mobomo commentedAddressed feedback.
Some of it though not sure gitlab was showing old versions.
Comment #17
smustgrave CreditAttribution: smustgrave at Mobomo commentedComment #18
larowlanOne minor comment now, and then I think this is ready
🤯 This one has been a real struggle to comprehend
Comment #19
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks! Will take care of it
Comment #20
smustgrave CreditAttribution: smustgrave at Mobomo commentedSince this has gone through a round of reviews and last feedback was a comment change think this is good to RTBC.
Comment #21
larowlan+1 RTBC
Comment #22
alexpottCommitted and pushed c8a07913a0 to 11.x and ebdf86b465 to 10.2.x. Thanks!
Backported to 10.2.x to keep tests aligned.
Comment #25
smustgrave CreditAttribution: smustgrave at Mobomo commentedThanks for all the help on this one everyone. Certainly was a confusing one.
Comment #26
larowlanYes definitely, thanks @alexpott for the eagle eyes