See title.

Had to do something that actually works and allows some progress :)

The default base class is converted to DrupalUnitTestBase, some that directly extended WebTestBase to UnitTestBase. A few tests require drupalGet()/drupalCreateUser() so I added a new DatabaseWebTestBase and used that for those. Removed the table creation from that, that never did anything because the web test *does* install the schema anyway. DrupalUnitTestBase does not for the $modules and does for enableModules() (still not sure why).

The complete database test group (32 test classes) now runs in a bit more than 2 minutes on my laptop, takes much much longer right now.

Can totally be postponed after feature freeze, but I wanted to get this started.

CommentFileSizeAuthor
#18 random_failure_in_entitytranslation.again_.png31.94 KBberdir
#16 database-unit-tests-1839134-16.patch11.31 KBberdir
#16 database-unit-tests-1839134-16-interdiff.txt1.86 KBberdir
#12 database-unit-tests-1839134-12.patch11.61 KBberdir
#8 drupal8.database-unit-test.8.patch9.5 KBsun
database-unit-tests.patch13.87 KBberdir

Comments

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Convert tests to (Database)UnitTestBase » Convert database tests to (Database)UnitTestBase

Status: Needs review » Needs work

The last submitted patch, database-unit-tests.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

database-unit-tests.patch queued for re-testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Not sure why UserCancelTest failed with a fatal error.

Looking at the testlog of that bot, looks like this saved 3-5 minutes on a single test run. That's.. nice.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Ok, so a local run on the (about 2x as fast as testbot, if not more) shows an improvement from 1m 8s for "php core/scripts/run-tests.sh --url http://d8/ --concurrency 8 Database" to 12s. And it's only 12s because it has to wait for the remaining web tests to finish up, the unit tests only took ~5-6s.

That does correspond with the estimated 2-3 minutes improvement on the testbot.

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
commented

Hm. Not too much of a fan of these ever-spiralling list of possible base test cases, but am a HUGE fan of reducing 1m 8s to 12s. ;)

Is it possible to just take the functions you need and roll them into DrupalUnitTestCase? Or else simply to avoid using helper functions in favour of direct API calls in those few tests?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

@webchick: DatabaseTestBase (which extends DrupalUnitTestBase) contains the module dependency definition and the setUp() + a few additional methods required by most database tests and nothing else.

DatabaseWebTestBase isn't really required but it would mean to duplicate a ~70 loc method into three or four different test classes (the same that is only useful for database tests) + setUp() and module dependencies or make it static on DatabaseTestBase and call it from there. I prefer having a base class for that.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
StatusFileSize
new drupal8.database-unit-test.8.patch9.5 KB

I actually prepared exactly this test group as a second showcase example for the original #1774388: Unit Tests Remastered™ issue.

As long as those database tests have an actual dependency on the database_test.module to supply the schema of test tables and example data, as well as facilitating hook_query_alter() functionality tests, these tests cannot really be pure unit tests, but need to use DrupalUnitTestBase instead.

The critical detail in most of these tests is that they do rely on database_test.module.

Attached patch is not 100% complete, but certainly gets us very far already.

Status: Needs review » Needs work

The last submitted patch, drupal8.database-unit-test.8.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

@sun: I know? You're patch is almost a subset of mine, we are both doing a DatabaseTestBase extends DrupalUnitTestBase and we both add a DatabaseWebTestBase for the tests that require drupalGet(). The differences are:

- I'm using UnitTestBase for a few separate tests that currently extend directly from WebTestBase, like the schema tests for example.
- I fixed the test failures that are reported in your patch.
- I moved the select test that requires a user (Note: the user doesn't need to be logged in AFAIK) to a separate web test, you made the user creation in work with DrupalUnitTestBase. Not sure what's better.
- You use enableModules() to install the database_test schema. Which is fine, but then those manual checks below aren't necessary because the schema is there. I have no idea why we are doing this right now, it is obviously not necessary when extending from WebTestBse either, so I removed it from DatabaseWebTestBase.
- You move the schema tests to Common, not sure why.

Maybe my patch can be extended with a few snippets but is otherwise further than yours, so please review (instead of stopping after you see the first UnitTestBase ;)).

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Yeah, sorry, it wasn't my intention to hi-jack this issue :-/ If there's anything in my patch that makes sense to you, let's merge it into yours! :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review
StatusFileSize
new database-unit-tests-1839134-12.patch11.61 KB

Ok, so here's my patch with some parts from sun's patch and some improvements of my of my own.

- Used sun's code to do the user creation. We need to check if we can actually make this test fail now and it's actually testing what's it supposed to.
- use enableModules() for database_test, nuked the installTables() code.
- made addSampleData() a static method, so that we can call it from DrupalWebTestBase.
- I'm actually not sure if we still need DrupalWebTest, just 3 classes using it and all it does is the set up call and $modules. Opinions?

Down to +65,-64. yay!

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

Does this convert the entire Database group? :) I've the impression I'm missing something? ;)

Only one remark on the patch:

+++ b/core/modules/system/lib/Drupal/system/Tests/Database/NextIdTest.php
@@ -7,12 +7,12 @@
+class NextIdTest extends UnitTestBase {

@@ -21,6 +21,13 @@ public static function getInfo() {
+  public function setUp() {
+    parent::setUp();
+    module_load_install('system');
+    $schema = system_schema();
+    db_create_table('sequences', $schema['sequences']);
+  }

If we need a module schema then we're implicitly relying on the module system, so all tests that need this should really use DrupalUnitTestBase. As soon as there is an Extensions service, DUTB will leverage that and UnitTestBase will break, because none of this information is available anymore.

Thus, let's switch this to DUTB and use installSchema() instead :)

sun’s picture

sun
Primary language German
Location Karlsruhe
commented 
+++ b/core/modules/system/lib/Drupal/system/Tests/Common/SchemaTest.php
@@ -26,7 +26,7 @@ public static function getInfo() {
       'name' => 'Schema API',
...
-      'group' => 'Database',
+      'group' => 'Common',

Ah, that's one change I'm missing :)

Essentially, I expect to be able to run

run-tests.sh --group Database

and get results in a few seconds. :)

-  function addSampleData() {
+  static function addSampleData() {

I don't quite get why we're changing this into a static method? :) Was that an earlier attempt?

-abstract class DatabaseTestBase extends WebTestBase {
-
-  /**
-   * Modules to enable.
-   *
-   * @var array
-   */
-  public static $modules = array('database_test');

I'm surprised that this patch passed without loading database_test module for most of those database tests. That said, the current definition of $modules in DUTB only loads the specified modules but does not install them. I've heard complaints about that not being what people expected though, so it's also possible that we might change that...

However, in turn, I'm a little confused how this patch can work, given that setUp() calls into addSampleData(), but without creating any database tables first...?

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Does this convert the entire Database group? :) I've the impression I'm missing something? ;)

Yes it does.

- Changing the NextIdTest to DUTB makes sense.

-      'group' => 'Database',
+      'group' => 'Common',

Ah, that's one change I'm missing :)

Essentially, I expect to be able to run

run-tests.sh --group Database

and get results in a few seconds. :)

Schema Tests are Unit tests now, they are fast (enough). Running the database testgroup on my desktop takes ~1minute through the UI, most of the time is however spent in the few remaining web tests. As said before, when using concurrency 8, I was able to get it down to ~12s and that was with one more web test than we have now.

EDIT: Haven't noticed before that schema tests are in the Common namespace. I think we should change that and move it to Database, not the other way round :)

- addSampleData() is static, because we call it statically from DatabaseWebTestBase, then we don't have to duplicate the code over there.

- The patch loads database_test with enableModules(array('database_test')), which installs the schema. Not that obvious in the diff but it's there at the end. No magic involved :)

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new database-unit-tests-1839134-16-interdiff.txt1.86 KB
new database-unit-tests-1839134-16.patch11.31 KB

Ok, converted NextIdTest to DUTB and moved SchemaTest into the Database namespace. That makes more sense to me. SchemaTest take 3s vor me, although this is a fast system. But I don't see "it's too slow" as a valid argument against not having it in Database :)

Status: Needs review » Needs work

The last submitted patch, database-unit-tests-1839134-16.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new random_failure_in_entitytranslation.again_.png31.94 KB

Another random test failure in EntityTranslationTest?!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

#16: database-unit-tests-1839134-16.patch queued for re-testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

#16: database-unit-tests-1839134-16.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, database-unit-tests-1839134-16.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

#16: database-unit-tests-1839134-16.patch queued for re-testing.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community

Thank you for staying on top of this!

I wrongly assumed earlier that SchemaTest would actually be about the Schema API; i.e., the stuff in includes/schema.inc. However, it is not; it's about the Schema support in the Database component. Thus, you're totally right in that the test should be moved into the Database tests. (And apparently, we do not seem to have dedicated test coverage for schema.inc... :-/)

Thanks!

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Title: Convert database tests to (Database)UnitTestBase » Convert database tests to (Drupal)UnitTestBase

Oh, I think now I know why sun was confused..

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks.

Status: Fixed » Closed (fixed)

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