Problem/Motivation

Unit tests should not require a database connection, but some in the suite currently do, therefore CI jobs enable the database services.

Steps to reproduce

Proposed resolution

Move the jobs that require a database away from the Unit testsuite, and drop the database service from the CI jobs.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3477529

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:

About issue forks

Comments

mondrake created an issue. See original summary.

mondrake opened merge request !9669

mondrake’s picture

mondrake
Location 🇮🇹
commented
Issue summary: View changes
mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Needs review
catch’s picture

catch
he/him
Primary language English
commented

This looks pretty reasonable to me. I was slightly concerned whether there might be a test where kernel test could 'hide' what might otherwise be a test failure for a uni test, but none of the tests look like that would be an issue.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Well, the Component*Test are clearly misplaced Kernel test - they extend from ComponentKernelTestBase which extends from KernelTestBase. Should really not be under unit test namespaces.

TestSiteApplicationTest requires a CI database service, and should be run for each one of the core databases. Moving to Kernel seems the correct step.

Once we remove the need for the CI database service, Unit tests could be run only once and not repeated for each database.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Rebased

smustgrave’s picture

smustgrave commented

Will this have any impact on contrib tests? May be a dumb question.

mondrake’s picture

mondrake
Location 🇮🇹
commented

No, contrib tests are not organised in suites; each run include all of the test types. At least in our test setup on GitLabCI. This is only relevant for core CI testing AFAICS.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Will go out on a limb then and mark it.

catch closed merge request !9669

  • catch committed 39259e30 on 11.x 
    Issue #3477529 by mondrake: [CI] Remove the 'with-database' requirement...
catch’s picture

catch
he/him
Primary language English
commented
Version: 11.x-dev » 10.4.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

Committed/pushed to 11.x, thanks!

I think we should probably backport this to 10.4.x given we'll be running tests against 10.4 for a while yet. Doens't cherry-pick cleanly to 11.0.x either but not worried about backport there.

mondrake’s picture

mondrake
Location 🇮🇹
commented

is backport required for 10.5.x now, 10.4.x, or both?

mondrake’s picture

mondrake
Location 🇮🇹
commented
Version: 10.4.x-dev » 10.5.x-dev

mondrake opened merge request !10021

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Patch (to be ported) » Needs review

  • catch committed d82f8059 on 10.5.x 
    Issue #3477529 by mondrake: [CI] Remove the 'with-database' requirement...

  • catch committed 02bd9df6 on 10.4.x 
    Issue #3477529 by mondrake: [CI] Remove the 'with-database' requirement...
catch’s picture

catch
he/him
Primary language English
commented

Committed/pushed to 10.5.x and went ahead and cherry-picked to the 10.4.x branch so that the (new) 11.1.x and 10.4.x branches have parity.

catch closed merge request !10021

mondrake’s picture

mondrake
Location 🇮🇹
commented
Version: 10.5.x-dev » 10.4.x-dev
Status: Needs review » Fixed

Status: Fixed » Closed (fixed)

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