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
- The Node module was "temporarily" added as a dependency to the Testing installation profile for #1373142: Use the Testing profile. Speed up testbot by 50%
- For the majority of web tests, Node module is unnecessarily installed, because they are testing their own module functionality.
Goal
- Improve test suite performance by removing the unnecessary Node module dependency from Testing profile.
Proposed solution
- Fix the
$modules
declaration of all tests that depend on Node module but do not specify it yet. - Let
node_install()
automatically grant the'access content'
permission for the built-in anonymous/authenticated user roles.
Comment | File | Size | Author |
---|---|---|---|
#50 | test.node_.50-interdiff.txt | 568 bytes | Berdir |
#50 | test.node_.50.patch | 55.96 KB | Berdir |
#47 | test.node_.47-interdiff.txt | 785 bytes | Berdir |
#47 | test.node_.47.patch | 55.4 KB | Berdir |
#45 | test.node_.45-interdiff.txt | 3.44 KB | Berdir |
Comments
Comment #1
sunKick-start.
Comment #2
Anonymous (not verified) CreditAttribution: Anonymous commentedyay.
Comment #4
sunSo this issue will get a bit more tricky.
Attached (2nd) patch shows that modules, which are not installed by the profile, (obviously) cannot provide any default configuration. Since there is no default configuration, all tests that currently assume that the default configuration exists, are failing.
"Default configuration" actually means "default configuration within the scope of tests" only.
This particular default configuration for Node module permissions basically has to be copied into every test that installs Node module. It cannot be put into node_install(), because we never ever auto-configure user permissions during installation of a module for security reasons.
This means we're borderline circling back into the discussion of #913086: Allow modules to provide default configuration for running tests - i.e., all modules need to be able to provide a default configuration that can be quickly + easily incorporated for testing.
In turn, this means:
Comment #6
sunTaking a different direction with #1768758: Change default profile for tests from Testing to Minimal now.
Comment #7
xjmRelated: #1811016: [meta] Decouple tests from Node module
Comment #8
sunTrying once more, using a slightly different approach.
Comment #9
sunSpin-off: #1865560: Remove user register config override from Testing profile
Comment #11
sunComment #13
sun#11: test.node_.11.patch queued for re-testing.
Comment #15
sunMerged HEAD.
Removed obsolete testing.install. :)
Comment #17
sunComment #18
BerdirWill try to re-roll and push this after /node is converted to a view, there is an issue that moves the access content permission into system.module and would make that part of the patch not strictly necessary but it probably still makes sense as we don't need to set up the default permissions then.
Comment #19
sunRelated: #1951526: Add a zero configuration installation profile
Comment #20
BerdirAs promised, simple re-roll.
Comment #22
sunRemoved 'access content' from route declarations.
Interdiff not possible, since patch is too old.
Comment #23
sunActually, let's split that into an own issue, since it's a bug on its own:
#2177093: Remove 'access content' permission from all test module routes
Comment #25
sunWe're back in business :)
Rebased against HEAD.
Comment #27
sunHrm. Trying to see whether the revised approach in attached patch is within the realms of possibilities?
Comment #28
larowlanAssuming testbot agrees.
In terms of granting access content when node is enabled, I don't see an issue with this. Previously if you were building a site you'd either start with minimal or standard anyway, unless you were making your own profile, in which case you'd (I'd) normally copy one of minimal and standard anyway.
And yeah, you'd be doing something special if you didn't want anonymous and authenticated users to access content, so that'd be something you would be explicitly testing, either manually or with automated testing. In other words, I think this change is fine.
Comment #30
sunVery interesting.
I first wanted to enable/install User module in the offending test, but then I asked myself:
The answer appears to be "Yes." — So attached patch accounts for that within node_install() instead.
→ User module may not necessarily be enabled/installed in DUTB tests.
Comment #31
sunWeirdo question in light of that:
Shouldn't the author/owner/uid base field for content entities actually be provided by User module?
If not available, save and assume 0 (or perhaps even NULL)?
Comment #32
sunTestbot fluke. Updated the issue summary.
Comment #33
BerdirI was confused how we went from 1k test fails to none with that simple change. Turns out, the latest patch is no longer removing the node dependency from testing.info.yml?
I assume that is *not* on purpose? This is the case since the patch in #25.
Comment #34
sunD'oh! Well spotted.
Comment #36
BerdirFixing some stuff. Hope @sun wasn't also working on it :)
Tons of tests also simply added the explicit dependency.
- BlockHtmlTest relied on the tools menu to show up, but that's not displayed as there are no links to show, switched to node.
- CommentNonNode shows very nicely that entity_id is currently misconfigured with a hardcoded node target type. There are already issues about this I think, is going to a bit bigger. Also don't want to interfere with the CommentInterface issue too much right now.
- ContactLinkTest: Fixed a default view perm to not use access content
- \Drupal\views_ui\Tests\UITestBase hardcodes node permissions, added node dependency there for now.
- CustomBlockPageViewTest specified custom permissions for viewing the custom block, but that is publicly accessible,without any permission check at all. The login is also bogus but don't want to change too much. Maybe this functionality is also broken, not sure how it is supposed to work?
- RestTestBase creates a node type by default but has not module dependencies at all. Also something to clean up elswhere IMHO, just added it...
- tracker didn't depend on node yet
- WriteRecordTest would be a nice DUBT test but needs node dependency as it writes to node_access.
- TaxonomyTestBase also created a node type but didn't depend on node.
Some tests I didn't figure out yet and mostly ignored the views related tests as the views base class was broken, adding the dependency there should fix most of those.
Comment #37
BerdirGo testbot.
Comment #38
tstoecklerWouldn't it make more sense to add a dependency of node module on user module? Since user module is required, this otherwise needs a very explicit comment that this is only for testing purposes.
Comment #39
Berdir@tstoeckler: That won't actually change anything, DUBT tests ignore module dependencies, otherwise all required modules would be enabled anyway, it's not necessary to depend on them. The test could be made dependant and I tend to agree that would be the cleaner solution, as cool as this might be ;)
Comment #41
BerdirFixing more tests, mostly adding node as dependency, removing permission where it's not needed.
- DependencyTest: changed a bit because node had to be enabled too.
- TokenReplaceTest: Would be easy to fix, but I'd rather to #1895018: Convert token API and node token tests to DrupalUnitTestBase to fix this.
- taxonomy.module uses permission 'access content' as the access check for the autocomplete route. Made visible by one of the views tests. That kind of means that taxonomy has to depend on node? All tests do now anyway..
-
Comment #43
BerdirAw, added the module to the wrong array for a bunch of the still failing views tests.
- Drupal\block\Tests\Views\DisplayBlockTest: I wasn't sure what the problem is, so I simply add the node dependency to make them pass:
- I switched the base field for $comment->entity_id back to an integer. I think that's the correct fix for now.
- TermTranslationUITest also failed because of the missing node dependency, simply because it gets an access denied on taxonomy/term/1. I added a dependency on node now and reverted the test changes, it obviously can't be used without node right now.
Unable to reproduce the DependencyTest fail.
Comment #45
BerdirOk, tracker.module was the only one that correctly called $comment->entity_id->target_id instead of ->value, the type change means that now only ->value works, so fixed that.
Also fixed DependencyTest again, I swear I had to do those changes on the desktop at home.
Also fixed AccessTest to add the node dependency, the test views there use node and while they should probably not, this is an easier fix. Didn't see that locally as it was a fatal error on the page callback and those aren't reported as 500 if you show erorrs :-/
The token test should have been fixed with the DUBT conversion that has been committed. This could be it...
Comment #47
BerdirUnbelievable.
Comment #48
sunThanks! RTBC if testbot agrees.
Comment #50
BerdirAww, the new FileDenormalizerTest shows that the rest.module default configuration currently relies on node.module and blows up if it doesn't.
That test should probably be DUBT, but I noticed that I could only run it successfully once, the second time, it did fail. Something with the cleanup in DUBT tests not working? I also can't run web tests right now, not sure if it that's related.
So just added the dependecy to the test for now. rest.module-- :(
Comment #51
sunComment #52
andypostAlso field_ui module test does not need node #2193609: No page title on manage display & form for non-default view mode
Comment #53
alexpottCommitted cdab74e and pushed to 8.x. Thanks!
I don't think this needs a change record since if someone is writing a test that requires node it will just fail now.
Comment #54
chx CreditAttribution: chx commentedRemoving node module from testing and breaking every D8 test depending on it definitely requires a separate change notice.
Comment #55
chx CreditAttribution: chx commentedSorry I didnt see in #53 claiming otherwise; the reason is that it's really not evident what happens and why your test fails.
Comment #56
BerdirAt whom would that change notice need to be targeted to? For 7.x to 8.x, we have http://drupal.org/node/1911318 (I just changed "almost-empty" to empty) about the testing install profile and we have https://drupal.org/node/1397856, which I also just added the reference to this issue.
So I think we have that covered, so the only additional thing we could do is have another 8.x -> 8.x change record that is about this specific change and references those two other change records. I'm a bit worried that adding too many of those will be confusing in the long-term.
Comment #57
chx CreditAttribution: chx commentedI guess we are not yet writing 8.x -> 8.x notices, OK.
Comment #58
sun