See #2735005: Convert all Simpletest web tests to BrowserTestBase (or UnitTestBase/KernelTestBase)

Currently two tests for the Simpletest UI use a WebTestBase test to check for proper discovery and running of tests in other profiles then the current profile. We can convert this to use a Kernel test instead and achieve the same coverage.

scope:

  • \Drupal\drupal_system_listing_compatible_test\Tests\SystemListingCompatibleTest
CommentFileSizeAuthor
#7 interdiff-2982740-4-7.txt1.73 KBApacheEx
#7 2982740-7.patch5.46 KBApacheEx
#4 interdiff-2982740-2-4.txt956 bytesApacheEx
#4 2982740-4.patch4.69 KBApacheEx
#2 2982740-2.patch4.33 KBApacheEx

Comments

ApacheEx created an issue. See original summary.

ApacheEx’s picture

ApacheEx commented
Status: Active » Needs review
StatusFileSize
new 2982740-2.patch4.33 KB

Here is a patch.

Status: Needs review » Needs work

The last submitted patch, 2: 2982740-2.patch, failed testing. View results

ApacheEx’s picture

ApacheEx commented
Status: Needs work » Needs review
StatusFileSize
new 2982740-4.patch4.69 KB
new interdiff-2982740-2-4.txt956 bytes

hmm, BTB or Kernel tests do not support assert message on success (at least it's not insert them into simpletest table as it is in \Drupal\simpletest\TestBase::assert()).

So, first column will be always empty. That's why 1 test fails.
https://www.dropbox.com/s/w9323ise2cvi450/Screenshot%202018-07-02%2010.0...

I would propose to change a bit assert for checking if test case is success.
Not sure if it's a good solution, any other ideas?

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Using the xpath to check for success makes sense to me.

This gives us the same coverage without using the webtest, nice!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work 
  /**
   * Use the Minimal profile.
   *
   * This test needs to use a different installation profile than the test which
   * asserts that this test is found.
   *
   * @see SimpleTestInstallationProfileModuleTestsTestCase
   *
   * @var string
   */
  protected $profile = 'minimal';

The problem here is that we're now not installing minimal and have lost one of the points of the test which is to ensure we can install other profile in tests located in other install profiles.

ApacheEx’s picture

ApacheEx commented
Status: Needs work » Needs review
StatusFileSize
new 2982740-7.patch5.46 KB
new interdiff-2982740-4-7.txt1.73 KB

Good catch @alexpott.
Also thanks @lendude for your review.
Here is updated patch.

lendude’s picture

lendude
Primary language Dutch
Location Amsterdam
commented
Status: Needs review » Reviewed & tested by the community 
/**
   * Attempt to enable a module from the Testing profile.
   *
   * This test uses the Minimal profile, but enables a module from the Testing
   * profile to confirm that a different profile can be used for running tests.
   *
   * @var array
   */
  public static $modules = ['drupal_system_listing_compatible_test'];

Dug into this test some more, and it turns out, this test no longer really tests that we can install a module outside the installed profile in the test, since there is a copy of the drupal_system_listing_compatible_test module in the system module. So if it can't find the version in the profile, it just installs the one in the system module.

The system version is used by \Drupal\Tests\system\Kernel\Common\SystemListingTest::testDirectoryPrecedence to check that modules placed in a profile override other versions of the same module. So we can't just remove the system version.

But that seems out of scope here, since that has probably been untested since 2013 or so, we can fix that in a follow up: #2984135: SystemListingCompatibleTest isn't testing what it says anymore

For this conversion: feedback has been addressed, looks good. \Drupal\simpletest\Tests\InstallationProfileModuleTestsTest conversion is handled in #2932909: Convert web tests to browser tests for Simpletest module

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Committed 6ae83e1 and pushed to 8.6.x. Thanks!

Thanks for opening the followup.

  • alexpott committed 6ae83e1 on 8.6.x 
    Issue #2982740 by ApacheEx, Lendude, alexpott: Convert \Drupal\...

Status: Fixed » Closed (fixed)

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