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.
Files: 
CommentFileSizeAuthor
#50 test.node_.50-interdiff.txt568 bytesBerdir
#50 test.node_.50.patch55.96 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 64,303 pass(es).
[ View ]
#47 test.node_.47-interdiff.txt785 bytesBerdir
#47 test.node_.47.patch55.4 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 64,176 pass(es), 8 fail(s), and 1 exception(s).
[ View ]
#45 test.node_.45-interdiff.txt3.44 KBBerdir
#45 test.node_.45.patch55.39 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 64,224 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#43 test.node_.43-interdiff.txt6.68 KBBerdir
#43 test.node_.43.patch52.81 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 64,163 pass(es), 16 fail(s), and 21 exception(s).
[ View ]
#41 test.node_.40-interdiff.txt20.07 KBBerdir
#41 test.node_.40.patch52.14 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 63,913 pass(es), 20 fail(s), and 9 exception(s).
[ View ]
#36 test.node_.35-interdiff.txt21.6 KBBerdir
#36 test.node_.35.patch32.07 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 62,890 pass(es), 80 fail(s), and 37 exception(s).
[ View ]
#34 interdiff.txt378 bytessun
#34 test.node_.34.patch10.46 KBsun
FAILED: [[SimpleTest]]: [MySQL] 54,577 pass(es), 391 fail(s), and 141 exception(s).
[ View ]
#30 interdiff.txt922 bytessun
#30 test.node_.30.patch10.09 KBsun
PASSED: [[SimpleTest]]: [MySQL] 63,744 pass(es).
[ View ]
#27 interdiff.txt4.12 KBsun
#27 test.node_.27.patch10.03 KBsun
FAILED: [[SimpleTest]]: [MySQL] 63,756 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
#25 test.node_.25.patch8.87 KBsun
FAILED: [[SimpleTest]]: [MySQL] 59,351 pass(es), 1,140 fail(s), and 20 exception(s).
[ View ]
#22 test.node_.22.patch28.2 KBsun
FAILED: [[SimpleTest]]: [MySQL] 42,498 pass(es), 1,149 fail(s), and 86 exception(s).
[ View ]
#20 test-node-1541298-20.patch33.78 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] 45,073 pass(es), 2,163 fail(s), and 382 exception(s).
[ View ]
#15 test.node_.15.patch34.92 KBsun
FAILED: [[SimpleTest]]: [MySQL] 45,458 pass(es), 1,805 fail(s), and 256 exception(s).
[ View ]
#15 interdiff.txt477 bytessun
#11 test.node_.11.patch34.86 KBsun
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch test.node_.11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#8 test.node_.8.patch1.65 KBsun
FAILED: [[SimpleTest]]: [MySQL] 44,986 pass(es), 1,145 fail(s), and 295 exception(s).
[ View ]
#4 platform.testing-node.4.patch2.42 KBsun
FAILED: [[SimpleTest]]: [MySQL] 31,889 pass(es), 1,709 fail(s), and 689 exception(s).
[ View ]
#1 platform.testing-node.1.patch1.41 KBsun
FAILED: [[SimpleTest]]: [MySQL] 31,796 pass(es), 1,800 fail(s), and 708 exception(s).
[ View ]

Comments

StatusFileSize
new1.41 KB
FAILED: [[SimpleTest]]: [MySQL] 31,796 pass(es), 1,800 fail(s), and 708 exception(s).
[ View ]

Kick-start.

yay.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new2.42 KB
FAILED: [[SimpleTest]]: [MySQL] 31,889 pass(es), 1,709 fail(s), and 689 exception(s).
[ View ]

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.

Status:Needs work» Closed (duplicate)

Status:Closed (duplicate)» Needs review
StatusFileSize
new1.65 KB
FAILED: [[SimpleTest]]: [MySQL] 44,986 pass(es), 1,145 fail(s), and 295 exception(s).
[ View ]

Trying once more, using a slightly different approach.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new34.86 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch test.node_.11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
  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.

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.

Status:Needs work» Needs review
StatusFileSize
new477 bytes
new34.92 KB
FAILED: [[SimpleTest]]: [MySQL] 45,458 pass(es), 1,805 fail(s), and 256 exception(s).
[ View ]

Merged HEAD.
Removed obsolete testing.install. :)

Status:Needs review» Needs work

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

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.

Status:Needs work» Needs review
StatusFileSize
new33.78 KB
FAILED: [[SimpleTest]]: [MySQL] 45,073 pass(es), 2,163 fail(s), and 382 exception(s).
[ View ]

As promised, simple re-roll.

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

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new28.2 KB
FAILED: [[SimpleTest]]: [MySQL] 42,498 pass(es), 1,149 fail(s), and 86 exception(s).
[ View ]

Removed 'access content' from route declarations.

Interdiff not possible, since patch is too old.

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.

Status:Needs work» Needs review
StatusFileSize
new8.87 KB
FAILED: [[SimpleTest]]: [MySQL] 59,351 pass(es), 1,140 fail(s), and 20 exception(s).
[ View ]

We're back in business :)

Rebased against HEAD.

Status:Needs review» Needs work

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

Status:Needs work» Needs review
StatusFileSize
new10.03 KB
FAILED: [[SimpleTest]]: [MySQL] 63,756 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new4.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.

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.

Status:Needs work» Needs review
StatusFileSize
new10.09 KB
PASSED: [[SimpleTest]]: [MySQL] 63,744 pass(es).
[ View ]
new922 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.

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

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

Testbot fluke. Updated the issue summary.

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.

StatusFileSize
new10.46 KB
FAILED: [[SimpleTest]]: [MySQL] 54,577 pass(es), 391 fail(s), and 141 exception(s).
[ View ]
new378 bytes

D'oh! Well spotted.

Status:Needs review» Needs work

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

StatusFileSize
new32.07 KB
FAILED: [[SimpleTest]]: [MySQL] 62,890 pass(es), 80 fail(s), and 37 exception(s).
[ View ]
new21.6 KB

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.

Status:Needs work» Needs review

Go testbot.

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

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

Status:Needs work» Needs review
StatusFileSize
new52.14 KB
FAILED: [[SimpleTest]]: [MySQL] 63,913 pass(es), 20 fail(s), and 9 exception(s).
[ View ]
new20.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.

Status:Needs work» Needs review
StatusFileSize
new52.81 KB
FAILED: [[SimpleTest]]: [MySQL] 64,163 pass(es), 16 fail(s), and 21 exception(s).
[ View ]
new6.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.

Status:Needs work» Needs review
StatusFileSize
new55.39 KB
FAILED: [[SimpleTest]]: [MySQL] 64,224 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new3.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.

Status:Needs work» Needs review
StatusFileSize
new55.4 KB
FAILED: [[SimpleTest]]: [MySQL] 64,176 pass(es), 8 fail(s), and 1 exception(s).
[ View ]
new785 bytes

Unbelievable.

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.

Status:Needs work» Needs review
StatusFileSize
new55.96 KB
PASSED: [[SimpleTest]]: [MySQL] 64,303 pass(es).
[ View ]
new568 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-- :(

Status:Needs review» Reviewed & tested by the community

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.

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.

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

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.

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

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

Status:Fixed» Closed (fixed)

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