Overview

Canvas leans heavily on kernel tests. That's fine - they're speedy! But they are very verbose, and very complex. That makes them hard to understand. We need to start simplifying them, whilst keeping the same robust level of test coverage.

Proposed resolution

A good first step would be to create a new base test class CanvasKernelTestBase, which:

  • ✅ Installs all Canvas dependencies (and their dependencies)
  • Installs necessary entity schemas → this would slow down the test suite: https://git.drupalcode.org/project/canvas/-/merge_requests/555#note_674401
  • ✅ Activates strict config schema checking by default, since Canvas makes extremely extensive use of validation
  • Imports core traits for creating specific entities or fields ← most Canvas kernel tests don't need this; require those that do need it to do this themselves.

✅ We should also change as many Canvas tests as possible to use this base class.

Out-of-scope:

  1. #3531679: Decouple kernel tests from `CanvasTestSetup`: painful to work on, and VERY slow
  2. #3572401: PHPCS: require CanvasKernelTestBase to be extended, forbid extending KernelTestBase

User interface changes

None.

Issue fork canvas-3563216

Command icon 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:

Comments

phenaproxima created an issue. See original summary.

wim leers’s picture

Component: … to be triaged » Code

Yes, please!

Are there more things you want to do on this MR, or is it ready for review?

wim leers’s picture

phenaproxima’s picture

Status: Active » Needs work

I gotta do more work on this.

wim leers’s picture

Assigned: phenaproxima » wim leers

And again at #3561392-22: Test Canvas on Drupal 11.3. I'll get this moving forward again.

wim leers’s picture

Issue summary: View changes
wim leers’s picture

Most of the remaining failures are in an area of known technical debt: the kernel tests reusing the fixture-intended-for-end-to-end-tests: #3531679: Decouple kernel tests from `CanvasTestSetup`: painful to work on, and VERY slow

Fixing all those would massively increase scope here. Instead, we should let that existing issue handling the conversion from KernelTestBase to CanvasKernelTestBase.

wim leers’s picture

Title: Add a base class for kernel tests » Add a base class for kernel tests to improve DX: `CanvasKernelTestBase`
Assigned: wim leers » phenaproxima
Priority: Normal » Major
Status: Needs work » Reviewed & tested by the community
Related issues:

A dozen or so cannot be updated, because #3531679: Decouple kernel tests from `CanvasTestSetup`: painful to work on, and VERY slow needs to happen first for them.

But other than that: all done! And it is very high impact:

+322, −992

(not to mention every future kernel test will be simpler)

I continued on the path that @phenaproxima started, but I now authored >50% of the MR, so asking him to sign off.

phenaproxima’s picture

Approved. That is indeed high-impact and a good step in the direction of long-term maintainability.

I think, if I have any outstanding concerns here, is that it's not clear why some tests enable some of the modules in Canvas's dependency, tree, but not all. Culling those $modules arrays even further is probably a good piece of clean-up for another issue. We also need to be sure that we explicitly document the limitations of CanvasKernelTestBase:

  • We need to be very clear that no entity type schemas are installed for you, and why we don't do that.
  • We should explain when this base class should be used, and when it shouldn't.

We might want to bring in certain helpful traits always, just to reduce friction and improve readability. Stuff like ContentTypeCreationTrait, MediaTypeCreationTrait, and so on.

Nonetheless, these don't need to happen right here, right now. Simply standardizing on one class is a big help.

wim leers’s picture

To ensure we don't regress and automatically provide this as feedback: #3572401: PHPCS: require CanvasKernelTestBase to be extended, forbid extending KernelTestBase.

wim leers’s picture

Issue summary: View changes
wim leers’s picture

Assigned: phenaproxima » Unassigned
StatusFileSize
new442.97 KB
new462.1 KB

Thanks, @phenaproxima!

I think, if I have any outstanding concerns here, is that it's not clear why some tests enable some of the modules in Canvas's dependency, tree, but not all.

WDYM? I just double-checked and the only dependency (direct or indirect) of Canvas that is missing is path_alias

We also need to be sure that we explicitly document the limitations of CanvasKernelTestBase:

  • We need to be very clear that no entity type schemas are installed for you, and why we don't do that.
  • We should explain when this base class should be used, and when it shouldn't.

The first bit is documented since https://git.drupalcode.org/project/canvas/-/merge_requests/555/diffs?com...

The second bit I'll document.

We might want to bring in certain helpful traits always, just to reduce friction and improve readability. Stuff like ContentTypeCreationTrait, MediaTypeCreationTrait, and so on.

I personally prefer explicitness over magic, and of the >60 tests using CanvasKernelTestBase (which are all passing ATM), less than a third actually need to install various entity schemas. See attached screenshots.

If we can confirm through manual testing that installing all entity schemas always does not slow the kernel tests down significantly, then I'm game. (Or, perhaps alternatively: a protected $installEntitySchemas = FALSE boolean that tests can override!)

phenaproxima’s picture

We need to be very clear that no entity type schemas are installed for you, and why we don't do that.

The first bit is documented since https://git.drupalcode.org/project/canvas/-/merge_requests/555/diffs?com...

Well...not quite. It says it installs the dependencies, but unless you're very familiar with kernel testing, you might assume that the entity schemas are installed too (as is done in functional tests). As you said, it is better to be explicit about this than to assume whoever is reading the comment knows the quirks.

wim leers’s picture

Well...not quite.

Yep, you were right — fixed that literally while you wrote that: https://git.drupalcode.org/project/canvas/-/merge_requests/555/diffs?com... 😄

Based on your feedback in #11, I also made sure that the new base class installs the path_alias module. But doing that triggered a whole avalanche of test failures, mostly because of \Drupal\filter\FilterPermissions::permissions() generating a URL to the permission's text format, which performs a path alias lookup 🙄
Still, it's only needed on <50% of the CanvasKernelTestBase-powered kernel tests!

Let's ship this, and consider expanding what the base class does in future issues. This MR is the hard one. Next ones will be simpler.

  • wim leers committed 3d50837d on 1.x
    chore(Project management): #3563216 Add a base class for kernel tests to...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed

Done!

See you next in:

  1. #3572401: PHPCS: require CanvasKernelTestBase to be extended, forbid extending KernelTestBase — to avoid regressing
  2. #3531679: Decouple kernel tests from `CanvasTestSetup`: painful to work on, and VERY slow — to finish what this started and address technical debt we've been carrying for >1 year

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

wim leers’s picture

Issue tags: +technical debt

Status: Fixed » Closed (fixed)

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