Some tests do not need a child Drupal to meddle with. Two samples included.

CommentFileSizeAuthor
#48 drupal_unit_test_case.patch15.18 KBchx
#47 drupal_unit_test_case.patch15.4 KBchx
#46 drupal_unit_test_case.patch15.61 KBchx
#45 drupal_unit_test_case.patch15.78 KBchx
#44 drupal_unit_test_case.patch15.09 KBchx
#42 drupal_unit_test_case.patch15.32 KBchx
#40 drupal_unit_test_case.patch13.24 KBchx
#27 drupal_unit_test_case.patch12.26 KBchx
#25 drupal_unit_test_case.patch12.83 KBchx
#24 drupal_unit_test_case.patch12.64 KBchx
#22 drupal_unit_test_case.patch12.35 KBchx
#21 drupal_unit_test_case.patch12.35 KBchx
#12 drupal_unit_test_case.patch12.13 KBchx
#10 drupal_unit_test_case.patch10.7 KBchx
#4 simpletest_speedup.patch5.86 KBchx
#1 simpletest_speedup.patch5.86 KBchx
simpletest_speedup.patch5.7 KBchx

Comments

chx’s picture

chx commented
StatusFileSize
new simpletest_speedup.patch5.86 KB

catch asked for more documentation. My pleasure, sir.

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

chx commented
Status: Needs work » Needs review

If i clock my CPU enough to time this i get 4s vs 0s...

chx’s picture

chx commented
StatusFileSize
new simpletest_speedup.patch5.86 KB

I was careless in the condition and it failed when there were no modules to be installed.

dries’s picture

dries commented

Personally, I think it is non-trivial to figure out whether a test writes to the database. Even a simple test can call some functions that indirectly cause a database write operation; e.g. a function has a watchdog call.

How many tests would benefit from this? Right now, it is hard to know whether we would benefit from introducing a special case. I prefer not to introduce special cases unless it brings significant improvements. Any thoughts or estimates on that?

Another idea could be to make this automatic. Install a listener on mutator queries, and install a new copy of Drupal upon the first insert or update.

chx’s picture

chx commented

See above: if you want to run a smaller test or two only the difference is colossal. And while it might not be easy to figure out when to use this, it's optional and if you do not use it, no harm done. I do not really think we want to mess with DBTNG just to allow such a simple feature.

catch’s picture

catch
he/him
Primary language English
commented

As long as this is documented clearly enough that it only gets used when it should be I think we're fine - and I think chx's comments are sufficient for that. It takes so long to run tests right now (I haven't run the full test suite locally for more than a month), that anything we can do to improve it is great.

cwgordon7’s picture

cwgordon7 commented

I think it would be cleaner to use this as an opportunity to separate DrupalWebTestCase into a DrupalWebTestCase and a DrupalUnitTestCase, both of which extend a base class (DrupalTestCase), which contains the methods common to both classes.

Another thought, why not just disconnect the database entirely for unit tests? Ideally, unit tests should not depend on anything in the database in order to function properly.

chx’s picture

chx commented

Disconnecting the db was my idea :) and can be achieved imo by changing the global $database and running a db_set_active to a connection that is guaranteed not to work.

chx’s picture

chx commented
StatusFileSize
new drupal_unit_test_case.patch10.7 KB

Here we come. I only mess with the db_prefix -- its a time honored way to make sure the DB is not available. If a test like this tries to access the database it will get some nice table does not exist exceptions from DBTNG. The patch is mostly copypaste.

damien tournoud’s picture

damien tournoud commented

I don't really see the use of that, it's simple enough just to override setUp() in your class if you are *really*, *really* confident that you don't need the database, nor to touch the registry or anything.

chx’s picture

chx commented
StatusFileSize
new drupal_unit_test_case.patch12.13 KB

Damien, how many tests should I convert so that you see that this is a common enough case?

damien tournoud’s picture

damien tournoud commented

#10 and #12 makes sense to me. But this will need a lot of documentation, in order to make the constraints cristal clear.

chx’s picture

chx commented

Test case for Drupal unit tests. These tests can not access the database nor files.

Might be a bit terse but care to suggest more comments, then? To me it sounds like the restrictions are clearly states.

chx’s picture

chx commented

Also note that my added safety check in simpletest.module needs to be either removed or fixed -- though the tests are green there are only 2K+ instead of the 10K+ as there should be. As we want to write more comments anyways, I keep this at CNR.

dries’s picture

dries commented

The last version of this patch makes for a much better design -- it is certainly less of a hack now. Like it much better.

A number of functions only make sense for DrupalWebTestCase, and not for DrupalUnitTestCase. For example, drupalCreateUser(). Should we move these from the base class, DrupalTestCase, to DrupalWebTestCase?

It might also make sense to rename drupal_web_test_case.php -- but we can do that in a quick follow-up patch.

chx’s picture

chx commented

Dries, yes we want to rename. I do not know what functiions are you talking of. The base class contains: __construct, assert, getAssertionCall, assertTrue, assertFalse, assertNull, assertNotNull, assertEqual, assertNotEqual, assertIdentical, assertNotIdentical, pass, fail, error, run, errorHandler, exceptionHandler, randomString, randomName and nothing else. These are IMO necessary in the base and nothing else is. drupalCreateUser have not been moved.

dries’s picture

dries commented

chx, you're right! Let's keep this moving forward then. :)

damien tournoud’s picture

damien tournoud commented

To clarifiy #13: we need to make clear that you cannot use any function that directly or indirectly touches the database. That includes watchdog(), drupal_function_exists(), module_invoke_all(), module_implements(), t() if locale is enabled, etc.

boombatower’s picture

boombatower commented

Hey! DrupalUnitTestCase is back!??! woha...

Seems to have the same concept as before and would finally make DrupalWebTestCase name useful again.

chx’s picture

chx commented
StatusFileSize
new drupal_unit_test_case.patch12.35 KB
chx’s picture

chx commented
StatusFileSize
new drupal_unit_test_case.patch12.35 KB

The above was Damien's comment enrolled into the Doxygen this one is whitespace fixes.

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Needs work

This cannot flight as-is: assertTrue(), assertFalse() and assertNull all can call t(), if no explicit message is passed to them. We either need to make t() work in the unit test environment (for example by removing the locale module from module_list()), or we need to remove those calls.

chx’s picture

chx commented
Status: Needs work » Needs review
StatusFileSize
new drupal_unit_test_case.patch12.64 KB

Not hard to do.

chx’s picture

chx commented
StatusFileSize
new drupal_unit_test_case.patch12.83 KB

More comments and a bit nicer code.

damien tournoud’s picture

damien tournoud commented

Dear testbot, please RTBC this for me if it comes back clean.

chx’s picture

chx commented
StatusFileSize
new drupal_unit_test_case.patch12.26 KB

Without the kitten-killing , unnecessary change in simpletest.module

dries’s picture

dries commented

I'm worried about the t() hack that we just added. It sounds like it could be the first hack of many hacks ...

If we want to speed up testing, it might be better to remove t() functions from assertXXX().

I'd prefer to keep this hack-free ...

dries’s picture

dries commented

Just to clarify -- I'd love to make the tests faster, but I don't want to obsess about it. Clean code is more important than fast tests, so I want us to optimize for clean code here.

chx’s picture

chx commented
Assigned: chx » Unassigned

I let others figure out why there are so few tests after the patch and either convince you to that fast tests are necessary or move t() to the result display. I am done here.

dries’s picture

dries commented

I don't understand this part of your comment: "I let others figure out why there are so few tests after the patch". Please be a bit more detailed. Thanks!

damien tournoud’s picture

damien tournoud commented
Status: Needs review » Needs work

@Dries:

drupal_unit_test_case.patch  Passed: 2359 passes, 0 fails, 0 exceptions

That's not a lot of assertions ;)

catch’s picture

catch
he/him
Primary language English
commented

We should kill t() from assertions anyway, I can't imagine anyone ever wanting to translate them, and will they fill up the {locale} table?

cwgordon7’s picture

cwgordon7 commented

How would Drupal know what language to translate it into if we're on a test database? Using t() should only be a problem for unit test assertions.

It also looks like it is not running tests outside of the simpletest/tests folder, though I have no idea why.

boombatower’s picture

boombatower commented

@chx: The reason the tests don't run properly is that assert() is a private method and cannot be declared that way when used by subclasses.

chx’s picture

chx commented

That needs to protected then.

boombatower’s picture

boombatower commented

@chx: indeed, was responding per #30.

It seems odd to include DrupalTestCase, DrupalUnitTestCase, and DrupalWebTestCase in a file named drupal_web_test_case.php

Perhaps drupal_test_case.inc?

chx’s picture

chx commented

As noted earlier we want to rename this file later.

dries’s picture

dries commented

If we rename the file (later!), let's also get rid of 'drupal_'. At some point, we should rename SimpleTest to Test or Testing, IMO.

I agree that we should probably nuke the t() functions from the assertions instead.

chx’s picture

chx commented
Assigned: Unassigned » chx
Status: Needs work » Needs review
StatusFileSize
new drupal_unit_test_case.patch13.24 KB

Now that locale disable is in, we can do more.

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

chx commented
Status: Needs work » Needs review
StatusFileSize
new drupal_unit_test_case.patch15.32 KB

I am down to 4 fails with this code. The problem is that despite my efforts, 

array (
  'message' => 'Created role of name: s365383s728225D406nUCv, id: 3',
  'type' => 'Role',
  'file' => 'drupal_web_test_case.php',
  'line' => '821',
  'function' => 'DrupalWebTestCase->drupalCreateRole()',
  'status' => 'Pass',
);
array (
  'message' => 'Created permissions: access content',
  'type' => 'Role',
  'file' => 'drupal_web_test_case.php',
  'line' => '828',
  'function' => 'DrupalWebTestCase->drupalCreateRole()',
  'status' => 'Pass',
);
array (
  'message' => 'User created with name s365383s7282255Pkfc3Ro and pass cfGJh9xaBa',
  'type' => 'User login',
  'file' => 'drupal_web_test_case.php',
  'line' => '794',
  'function' => 'DrupalWebTestCase->drupalCreateUser()',
  'status' => 'Pass',
);
array (
  'message' => 'Invalid permission invalid permission.',
  'type' => 'Role',
  'file' => 'drupal_web_test_case.php',
  'line' => '856',
  'function' => 'DrupalWebTestCase->checkPermissions()',
  'status' => 'Fail',
);

these still come from DWTC instead of the stub test they should.

Edit: the dump is from simpletest.test assertAssertion.

Status: Needs review » Needs work

The last submitted patch failed testing.

chx’s picture

chx commented
Status: Needs work » Needs review
StatusFileSize
new drupal_unit_test_case.patch15.09 KB

This was not easy. PHP below 5.3 does not see private properties with property_exists. It does see methods with method_exists but even if a method is defined as private it exists in the children. But! constants to the rescue.

chx’s picture

chx commented
StatusFileSize
new drupal_unit_test_case.patch15.78 KB

Added the design motivation in the doxygen. No code change.

chx’s picture

chx commented
StatusFileSize
new drupal_unit_test_case.patch15.61 KB

Removed some comment duplication.

chx’s picture

chx commented
StatusFileSize
new drupal_unit_test_case.patch15.4 KB

Changed to a property per DamZ's suggestion. Seems less hackish. Also, we could evaluate our tests to see which utility classes needs to be skipped, it's just a matter of copy-pasting the constructor around.

chx’s picture

chx commented
StatusFileSize
new drupal_unit_test_case.patch15.18 KB

More code cleanup thanks to DamZ. This is getting close.

damien tournoud’s picture

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

It looks good, and it has been green lighted by the bot, let's get this in ;)

dries’s picture

dries commented

Will commit this tomorrow morning -- feel free to start updating more tests in the mean time. We might discover new things as we do so.

dries’s picture

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

Committed. Thanks!

boombatower’s picture

boombatower commented

Since this re-introduced the DrupalUnitTest case, #266555: Rename DrupalWebTestCase to DrupalTestCase? is no longer useful.

Status: Fixed » Closed (fixed)

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