Problem

  1. 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%
  2. 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

  1. Fix the $modules declaration of all tests that depend on Node module but do not specify it yet.
  2. Let node_install() automatically grant the 'access content' permission for the built-in anonymous/authenticated user roles.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sun’s picture

Kick-start.

Anonymous’s picture

yay.

Status: Needs review » Needs work

The last submitted patch, platform.testing-node.1.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
2.42 KB

So 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:

  1. we're either blocked on that issue, or
  2. we'd say KISS and bite the bullet and copypaste those lines into every test case that needs them.

Status: Needs review » Needs work

The last submitted patch, platform.testing-node.4.patch, failed testing.

sun’s picture

Status: Needs work » Closed (duplicate)
xjm’s picture

sun’s picture

Status: Closed (duplicate) » Needs review
FileSize
1.65 KB

Trying once more, using a slightly different approach.

sun’s picture

Status: Needs review » Needs work

The last submitted patch, test.node_.8.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
34.86 KB
  1. Fixed unnecessary 'access content' router path access conditions in test modules.
  2. Fixed missing Node module dependency in test classes.
  3. Fixed node_install() must not set user permissions.

Status: Needs review » Needs work
Issue tags: -Testing system

The last submitted patch, test.node_.11.patch, failed testing.

sun’s picture

Status: Needs work » Needs review

#11: test.node_.11.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Testing system

The last submitted patch, test.node_.11.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
477 bytes
34.92 KB

Merged HEAD.
Removed obsolete testing.install. :)

Status: Needs review » Needs work

The last submitted patch, test.node_.15.patch, failed testing.

sun’s picture

Issue tags: +Test suite performance
Berdir’s picture

Will 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.

sun’s picture

Berdir’s picture

Status: Needs work » Needs review
FileSize
33.78 KB

As promised, simple re-roll.

The last submitted patch, test-node-1541298-20.patch, failed testing.

sun’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
28.2 KB

Removed 'access content' from route declarations.

Interdiff not possible, since patch is too old.

sun’s picture

Actually, 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

Status: Needs review » Needs work

The last submitted patch, 22: test.node_.22.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
8.87 KB

We're back in business :)

Rebased against HEAD.

Status: Needs review » Needs work

The last submitted patch, 25: test.node_.25.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
10.03 KB
4.12 KB

Hrm. Trying to see whether the revised approach in attached patch is within the realms of possibilities?

  1. Moved granting of default 'access content' permissions into node_install().
  2. Removed irrelevant change.
larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Assuming 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 27: test.node_.27.patch, failed testing.

sun’s picture

Status: Needs work » Needs review
FileSize
10.09 KB
922 bytes

Very interesting.

I first wanted to enable/install User module in the offending test, but then I asked myself:

Given the fact that many DUTB tests are passing, despite not enabling/installing User module, does that mean that Node module is able to operate without User module?

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.

sun’s picture

Weirdo 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)?

sun’s picture

Issue summary: View changes
Status: Needs work » Needs review
Parent issue: » #2006434: [meta] Speed up web tests

Testbot fluke. Updated the issue summary.

Berdir’s picture

I 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.

sun’s picture

FileSize
10.46 KB
378 bytes

D'oh! Well spotted.

Status: Needs review » Needs work

The last submitted patch, 34: test.node_.34.patch, failed testing.

Berdir’s picture

Fixing 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.

Berdir’s picture

Status: Needs work » Needs review

Go testbot.

tstoeckler’s picture

+++ b/core/modules/node/node.install
@@ -423,6 +423,18 @@ function node_schema() {
+  if (\Drupal::moduleHandler()->moduleExists('user')) {

Wouldn'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.

Berdir’s picture

@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 ;)

Status: Needs review » Needs work

The last submitted patch, 36: test.node_.35.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
52.14 KB
20.07 KB

Fixing 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..
-

Status: Needs review » Needs work

The last submitted patch, 41: test.node_.40.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
52.81 KB
6.68 KB

Aw, 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.

Status: Needs review » Needs work

The last submitted patch, 43: test.node_.43.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
55.39 KB
3.44 KB

Ok, 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...

Status: Needs review » Needs work

The last submitted patch, 45: test.node_.45.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
55.4 KB
785 bytes

Unbelievable.

sun’s picture

Assigned: sun » Unassigned
Status: Needs review » Reviewed & tested by the community

Thanks! RTBC if testbot agrees.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 47: test.node_.47.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
FileSize
55.96 KB
568 bytes

Aww, 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-- :(

sun’s picture

Status: Needs review » Reviewed & tested by the community
andypost’s picture

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 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.

chx’s picture

Priority: Normal » Major
Status: Fixed » Active
Issue tags: +Needs change record

Removing node module from testing and breaking every D8 test depending on it definitely requires a separate change notice.

chx’s picture

Sorry I didnt see in #53 claiming otherwise; the reason is that it's really not evident what happens and why your test fails.

Berdir’s picture

At 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.

chx’s picture

Priority: Major » Normal
Status: Active » Fixed
Issue tags: -Needs change record

I guess we are not yet writing 8.x -> 8.x notices, OK.

sun’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.