Problem/Motivation

1. There is no test coverage for theme's supporting *.config_translation.yml files on them.
2. The support for themes was disabled in #2123575-16: Fixes for tim.plunkett and dawehner review due to conflicts with unit testability. Figure out a way to make unit tests work with this.

Proposed resolution

1. Add coverage with a test theme.
2. Expert advice welcome! We can add our own wrapper class for list_themes() but that sounds pretty awful :/ We could also postpone theme support on core converting this to a service, but that also sounds awful.

Remaining tasks

Figure out fix for 2.

User interface changes

None.

API changes

None.

#2123575: Fixes for tim.plunkett and dawehner review

CommentFileSizeAuthor
#38 test-theme-38.patch4.26 KBGábor Hojtsy
#37 test-theme-only.patch1.79 KBGábor Hojtsy
#35 test-theme-35.patch6.05 KBGábor Hojtsy
#32 interdiff.txt543 bytesGábor Hojtsy
#32 test-theme-32.patch6.55 KBGábor Hojtsy
#28 test-theme-only.patch2.3 KBGábor Hojtsy
#24 test-theme-13.patch6.56 KBGábor Hojtsy
#21 test-theme-12.patch6.56 KBGábor Hojtsy
#19 test-theme-11.patch9.16 KBGábor Hojtsy
#19 interdiff.txt2.6 KBGábor Hojtsy
#17 test-theme-10.patch6.56 KBGábor Hojtsy
#17 interdiff.txt898 bytesGábor Hojtsy
#15 test-theme-9.patch6.11 KBGábor Hojtsy
#15 interdiff.txt587 bytesGábor Hojtsy
#12 test-theme-8.patch6.09 KBGábor Hojtsy
#12 interdiff.txt341 bytesGábor Hojtsy
#11 test-theme-7.patch6.42 KBGábor Hojtsy
#11 interdiff.txt4.04 KBGábor Hojtsy
#9 test-theme-6.patch7.58 KBGábor Hojtsy
#9 interdiff.txt946 bytesGábor Hojtsy
#8 interdiff.txt3.92 KBGábor Hojtsy
#8 test-theme-5.patch7.56 KBGábor Hojtsy
#6 test-theme-4.patch4.26 KBGábor Hojtsy
#6 interdiff.txt2.37 KBGábor Hojtsy
#4 test-theme-3.patch3.71 KBGábor Hojtsy
#4 interdiff.txt785 bytesGábor Hojtsy
#2 test-theme-2.patch2.94 KBGábor Hojtsy
#2 interdiff.txt576 bytesGábor Hojtsy
#1 test-theme.patch2.97 KBGábor Hojtsy
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Active » Needs review
FileSize
test-theme.patch2.97 KB

This should still fail on the unit tests due to the problem with lack of ways to mock list_themes() but hopefully will pass the theme .yml test that is newly added. Untested :)

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
interdiff.txt576 bytes
test-theme-2.patch2.94 KB

Fix info file end newline.

Status: Needs review » Needs work

The last submitted patch, test-theme-2.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
interdiff.txt785 bytes
test-theme-3.patch3.71 KB

We need to implement hook_system_theme_info() to expose this theme actually because its buried under a module.

Status: Needs review » Needs work

The last submitted patch, test-theme-3.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.37 KB
test-theme-4.patch4.26 KB

The test needs to log in with a user that can actually visit that page, and then fine :) The other changes in the interdiff are just info file cleanup stuff.

Status: Needs review » Needs work

The last submitted patch, test-theme-4.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
test-theme-5.patch7.56 KB
interdiff.txt3.92 KB

#2109287: Replace list_themes() with a service. exists and would allow us to properly fix that unit test in the best way. In the meantime we can work around that by injecting a custom wrapper for theme listing and mocking that. We need to register that as a service, so we can pass it on, so we can mock it, etc. This looks like maybe a bit too much just so we can keep unit testing the manager. Expert feedback welcome :)

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
interdiff.txt946 bytes
test-theme-6.patch7.58 KB

Missed variable name on docs.

Status: Needs review » Needs work

The last submitted patch, test-theme-6.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
interdiff.txt4.04 KB
test-theme-7.patch6.42 KB

So I forgot to add in the new service on the test constructor. Anyway, @dawehner pointed out I should use an easier method. Just subclass the class being tested and override a method where I put the global stuff :D So here you go. The ConfigMapperThemeDiscovery is also removed, not in the interdiff, sorry, screwed up there.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
interdiff.txt341 bytes
test-theme-8.patch6.09 KB

Should not reference the non-existent theme discovery service anymore :D

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Looks perfect if it passes the great testbot barrier.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, test-theme-8.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
interdiff.txt587 bytes
test-theme-9.patch6.11 KB

Need to have the signatures match obviously :) Still cannot run locally nicely, so not 100% sure it will be green.

Status: Needs review » Needs work

The last submitted patch, test-theme-9.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
interdiff.txt898 bytes
test-theme-10.patch6.56 KB

Need to mock drupal_get_path() too. Works for me locally now.

Status: Needs review » Needs work

The last submitted patch, test-theme-10.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
interdiff.txt2.6 KB
test-theme-11.patch9.16 KB

Now needs to be updated for #2048223: Add $account argument to AccessCheckInterface::access() method and use the current_user service, since the tests will not pass anymore due to difference in the interface. Heh :D

Status: Needs review » Needs work

The last submitted patch, test-theme-11.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
test-theme-12.patch6.56 KB

Committed the #19 interdiff only (to fix core related update needed at http://drupalcode.org/project/config_translation.git/commitdiff/8e3dbfee...).

Rerolled without that. It ran properly on my machine with a full phpunit pass. Not sure why is it not finding the class still. I'll need to update a core checkout, etc. later on. Meh. No time anymore today.

So this should be the same as #17 since I committed the access interface fix, which was truly unrelated.

Status: Needs review » Needs work

The last submitted patch, test-theme-12.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

Puh, this could be an issue with the way how contrib modules phpunit test are integrated into simpletest.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
test-theme-13.patch6.56 KB

Rerolled after the turbulent fails of core changes are now fixed on the module. No change.

Status: Needs review » Needs work

The last submitted patch, test-theme-13.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review

Could this be a PHP 5.3 problem? I'm running it locally on:

$ php -v
PHP 5.4.17 (cli) (built: Aug 25 2013 02:03:38)

and it passes fine:

$ vendor/bin/phpunit -v ../modules/config_translation/tests/Drupal/config_translation/Tests/ConfigMapperManagerTest.php 
PHPUnit 3.7.21 by Sebastian Bergmann.

Configuration read from /Users/gabor/Web/d8mi/core/phpunit.xml.dist

..........

Time: 0 seconds, Memory: 5.25Mb

OK (10 tests, 40 assertions)
Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

24: test-theme-13.patch queued for re-testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Issue summary: View changes
FileSize
test-theme-only.patch2.3 KB

Also here is a copy with just the added test theme that should still pass. We can get that committed in the interim, so the patch to review is smaller and less intimidating :)

Status: Needs review » Needs work

The last submitted patch, 24: test-theme-13.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review

28: test-theme-only.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 28: test-theme-only.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
test-theme-32.patch6.55 KB
interdiff.txt543 bytes

@timplunkett points out a small error in the test (which should not lead to the fail indicated by testbot though). Fixing that at least. If this only fails with the 5 fails on access() interface that is due to #2125349: Testbot not testing contrib 8.x with up to date 8.x core? This is derived from #24, the interim patch was just a test to get something simpler pass but it exhibited the same testbot bug from #2125349: Testbot not testing contrib 8.x with up to date 8.x core?.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review

Send for testbot.

Status: Needs review » Needs work

The last submitted patch, 32: test-theme-32.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
test-theme-35.patch6.05 KB

Rerolled patch since the above one did not apply anymore. (I committed the .info.yml file fix for the test module as an innocent commit to kick off testbot's happy green test following a testbot bug :D).

Status: Needs review » Needs work

The last submitted patch, 35: test-theme-35.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review
FileSize
test-theme-only.patch1.79 KB

Want to commit at least a logical part that in itself passes. The theme only should :D

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
FileSize
test-theme-38.patch4.26 KB

So now only the test remains. Easier to review, but will fail on testbot :/ Does not fail locally.

Status: Needs review » Needs work

The last submitted patch, 38: test-theme-38.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented

I just went to https://drupal.org/project/project_issue_file_review in order to search how contrib modules are tested:

  protected function test_list() {
    elseif (!($this->is_core_test())) {
      $args = array();
      // Not a core test?  Scan the sites/all/modules/modulename directory
      $this->log("Not a core test.  Scanning module directory.");
      $args += $this->findTestFiles($this->module_directory . '/' . $this->project_directory);
      return '--file ' . implode(',', $args);
    }
  }

  protected function findTestFiles($directory) {
    ...
    $ignore = array('.', '..', 'vendor');
    foreach (file_scan_directory($basedir . $directory, '\.php$', $ignore) as $file) {
      // '/Tests/' can be contained anywhere in the file's path (there can be
      // sub-directories below /Tests), but must be contained literally
      // (case-sensitive).
      if (strpos($file->filename, '/Tests/')) {
  }

so let's see what happens if you do the same on the shell:

sudo XDEBUG_CONFIG="idekey=PHPSTORM" -u www-data php ./scripts/run-tests.sh --url http://localhost/d8 --color --verbose --file /var/www/d8/modules/config_translation/tests/Drupal/config_translation/Tests/ConfigMapperManagerTest.php                     
Drupal test run
---------------

Tests to be run:
 - Configuration translation mapper manager (Drupal\config_translation\Tests\ConfigMapperManagerTest)
 -  (Drupal\config_translation\Tests\TestConfigMapperManager)

... there we go!

simpletest_script_get_test_list() parses that list, so all we have to check is whether the class is either extending TestBase or UnitTestCase, yeah!
Here is a core issue: #2129197: Testbot (run-tests.sh with --file) attempts to run non-test classes, fails

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented

The fix in #2129197: Testbot (run-tests.sh with --file) attempts to run non-test classes, fails works nicely, it looks great. RTBC-ed. Let's get that in, so we can get this in, so we can get the config translation in, so we can uncover more bugs :D

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review

38: test-theme-38.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 38: test-theme-38.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Needs review

38: test-theme-38.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 32: test-theme-32.patch, failed testing.

Gábor Hojtsy’s picture

Gábor Hojtsy
he/him
Primary language Hungarian
Location Hungary
CreditAttribution: Gábor Hojtsy commented
Status: Needs work » Fixed

Yay, committed #38!

Status: Fixed » Closed (fixed)

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