Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet’s picture

Status: Active » Needs review
FileSize
6.62 KB

Here's a rename.

dawehner’s picture

Given the code we could also convert this test into a proper unit test using vfs, see \Drupal\Tests\user\Unit\PermissionHandlerTest as example.

joelpittet’s picture

@dawehner I added some non-unit test stuff to it I think. #2409515: Updater::findInfoFile() lacks test coverage

I made this issue so the rename didn't look messy. Maybe you have a better place for that though? I'm horrible at finding places to put tests.

lauriii’s picture

Status: Needs review » Reviewed & tested by the community

I've struggled with this too. I was debugging why I cant run this inside phpstorm and after all figured out that its not a actual unit test. The patch still applies. This is a little bit disruptive because patches changing that test needs reroll but I still think this is worth doing.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: -Needs backport to D7

I agree with @dawehner let's convert this to a proper unit test.

joelpittet’s picture

Issue tags: +Novice

Thanks for committing the other issue. That shouldn't be too trick(I hope) as long as things are mockable and self contained it should work. IIRC, may have had a file_scan_directory() call in there that may have made this tricky to do...

I'll take this on in a while if nobody picks it up in the mean time.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
856 bytes

I know we are considering to put this back into an unit test, but I thought it would be good to have a reroll as a reference. Please set back to needs work once tests complete.

dawehner’s picture

What about converting it to a unit test instead? With vfs this should be totally be doable in a nice way!

hussainweb’s picture

@dawehner: Yes, I am looking into that. I just wanted to get this patch rerolled to have as a reference.

hussainweb’s picture

I have not used VFS here yet but this is a straight move to unit tests directory and changing it to use PHPUnit.

dawehner’s picture

Status: Needs review » Needs work
  1. index ae52201..3bd15e5
    --- a/core/modules/system/src/Tests/Extension/InfoParserUnitTest.php
    
    --- a/core/modules/system/src/Tests/Extension/InfoParserUnitTest.php
    +++ b/core/modules/system/tests/src/Unit/Extension/InfoParserUnitTest.php
    

    IMHO we should move it to the core unit tests

  2. +++ b/core/modules/system/tests/src/Unit/Extension/InfoParserUnitTest.php
    @@ -76,13 +76,13 @@ public function testInfoParser() {
         }
         catch (InfoParserException $e) {
    

    We should use @expectedException ...

The last submitted patch, 10: infoparserunittest_web-2490388-10.patch, failed testing.

hussainweb’s picture

Status: Needs work » Needs review
FileSize
5.17 KB

Just moved the file again as per #11.1. I moved the second test in this class to a new file. I am trying out VFS with a small change first and then will go on to convert the whole test.

Status: Needs review » Needs work

The last submitted patch, 13: infoparserunittest_web-2490388-13.patch, failed testing.

hussainweb’s picture

Status: Needs review » Needs work

The last submitted patch, 15: infoparserunittest_web-2490388-15.patch, failed testing.

dawehner’s picture

@hussainweb Thank you for the effort!

hussainweb’s picture

Status: Needs work » Needs review
FileSize
9.67 KB

So, it turns out that the test for Updater is not easy with PHPUnit. We need to mock a static method (Updater::findInfoFile()) to get it to work but that is not possible anymore. I moved it back to the regular test. Once this patch turns green, we could rename the file to clarify it's function and remove UnitTest from the name.

The other test now uses vfs. The test method is now split into four different methods.

dawehner’s picture

OOH:

  /**
   * Tests project and child project showing correct title.
   *
   * @see https://drupal.org/node/2409515
   */
  public function testGetProjectTitleWithChild() {
    // Get the project title from it's directory. If it can't find the title
    // it will choose the first project title in the directory.
    $directory = \Drupal::root() . '/core/modules/system/tests/modules/module_handler_test_multiple';
    $title = Updater::getProjectTitle($directory);
    $this->assertEqual('module handler test multiple', $title);
  }

Yeah that does not belong into the unit test for this class. I'd suggest to maybe keep it in its current position.

hussainweb’s picture

Renaming the file and removing an unused use.

dawehner’s picture

+++ b/core/modules/system/src/Tests/Extension/UpdaterTest.php
@@ -0,0 +1,37 @@
+ * Contains \Drupal\system\Tests\Extension\InfoParserUnitTest.
+ */

Old doc ...

Ankit Agrawal’s picture

Ankit Agrawal’s picture

The last submitted patch, 22: infoparserunittest_web-2490388-21.patch, failed testing.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Nice work!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Nice work everyone. Committed c17a82f and pushed to 8.0.x. Thanks!

  • alexpott committed c17a82f on 8.0.x
    Issue #2490388 by hussainweb, Ankit Agrawal, joelpittet, dawehner:...

Status: Fixed » Closed (fixed)

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