Follow-up to #2458003: Port MigrateSourceCSV to Drupal 8 migrate source plugin

MigrateSourceCSV should be a plugin for migrate that extends the migrate base classes for migrate source.

The patch in #2458003: Port MigrateSourceCSV to Drupal 8 migrate source plugin didn't include the tests for CSVFileObject. I'm sure I missed adding that file This includes the missing tests.

Comments

heddn’s picture

heddn’s picture

Title: Port MigrateSourceCSV to Drupal 8 migrate source plugin » Port MigrateSourceCSV to Drupal 8 missing tests
heddn’s picture

Version: » 8.x-1.x-dev
FileSize
3.39 KB

Re-uploading the patch from #1 to get the testbot to see it. Since it was originally uploaded the project was promoted and testing enabled.

heddn’s picture

FileSize
3.39 KB
None View

Try it again.

phenaproxima’s picture

Status: Needs review » Needs work

Found a few minor issues with the test, but otherwise I think this looks great.

  1. +++ b/migrate_source_csv/tests/src/Unit/CSVFileObjectTest.php
    @@ -0,0 +1,117 @@
    +  public function create() {
    

    Why use the @test annotation? Won't renaming the method testCreate() work just as well?

  2. +++ b/migrate_source_csv/tests/src/Unit/CSVFileObjectTest.php
    @@ -0,0 +1,117 @@
    +    $csv_file_object = new CSVFileObject($this->happyPath);
    

    This is done, it seems, in every test method. Should probably be done in a setUp() method.

  3. +++ b/migrate_source_csv/tests/src/Unit/CSVFileObjectTest.php
    @@ -0,0 +1,117 @@
    +    $this->assertInstanceOf('\Drupal\migrate_source_csv\CSVFileObject', $csv_file_object);
    

    Nit: We can use CSVFileObject::class for better readability.

  4. +++ b/migrate_source_csv/tests/src/Unit/CSVFileObjectTest.php
    @@ -0,0 +1,117 @@
    +    $flags = CSVFileObject::READ_CSV | CSVFileObject::READ_AHEAD | CSVFileObject::DROP_NEW_LINE | CSVFileObject::SKIP_EMPTY;
    +    $this->assertEquals($flags, $csv_file_object->getFlags());
    

    Should be assertSame().

  5. +++ b/migrate_source_csv/tests/src/Unit/CSVFileObjectTest.php
    @@ -0,0 +1,117 @@
    +   * @test
    +   *
    +   * @covers ::getHeaderRowCount
    +   * @covers ::setHeaderRowCount
    +   */
    +  public function headerRowCount() {
    

    Ditto about using @test versus calling the method testHeaderRowCount().

  6. +++ b/migrate_source_csv/tests/src/Unit/CSVFileObjectTest.php
    @@ -0,0 +1,117 @@
    +  public function countLines() {
    

    Since this uses setHeaderRowCount(), it might be prudent to add a @depends headerRowCount annotation.

heddn’s picture

Status: Needs work » Needs review
FileSize
3.21 KB
3.65 KB
None View

5.1/5.5
There's no core or contrib standard about @test annotations, at least that I could find. Using that annotation, it is really easy to tell what is under test, cause you can name it the same thing as the class under test.

5.2,5.3,5.4,5.6 are all addressed.

phenaproxima’s picture

Few other things -- sorry I didn't mention these in the last go-around.

  1. +++ b/migrate_source_csv/tests/src/Unit/CSVFileObjectTest.php
    @@ -18,6 +18,16 @@ use org\bovigo\vfs\content\LargeFileContent;
    +   * @var \Drupal\migrate_source_csv\CSVFileObject CSV file object.
    +   */
    +  protected $csv_file_object;
    

    Need a new line after the class name, and the property itself should use camel case in accordance with the Drupal coding standards.

  2. +++ b/migrate_source_csv/tests/src/Unit/CSVFileObjectTest.php
    @@ -25,10 +35,23 @@ class CSVFileObjectTest extends CSVUnitTestCase {
    +  public function setHeaderRowCount() {
    +    $expected = 2;
    +    $this->csv_file_object->setHeaderRowCount($expected);
    +
    +    return $this->csv_file_object->getHeaderRowCount();
    

    This doesn't assert anything...

  3. +++ b/migrate_source_csv/tests/src/Unit/CSVFileObjectTest.php
    @@ -56,10 +76,9 @@ class CSVFileObjectTest extends CSVUnitTestCase {
    +    $this->csv_file_object->setHeaderRowCount(1);
    +    $actual = $this->csv_file_object->count();
     
         $this->assertEquals($expected, $actual);
    

    This can all be one line.

heddn’s picture

FileSize
2.45 KB
3.68 KB
None View

7.1: I think this is now addressed.
7.2: I cannot test the setter unless I call the getter, which is what I'm doing. It's depended on by the getter now, per your earlier feedback in #5.
7.3: I don't understand the feedback.

KarenS’s picture

I see attempts to test this code. Testing won't work for a submodule, see https://www.drupal.org/node/2499239 for a discussion. So the testbot can't test this. Locally testing won't work even in a modules/contrib folder. Every module with tests has to be top-level in the modules folder. Moving the module out reveals that there's a failing test in the current code (before this patch) from the attempt to use protected method getIterator().

heddn’s picture

Project: Migrate Plus » Migrate Source CSV
Version: 8.x-1.x-dev »
Component: Source plugins » Code

  • heddn committed e91db27 on 8.x-1.x
    Issue #2524434 by heddn: Port MigrateSourceCSV to Drupal 8 missing tests
    
heddn’s picture

Version: » 8.x-1.x-dev
Status: Needs review » Fixed

Fixed up the unit tests. There was a broken part in the class under test around automatically generating column names from the header row if no column names were provided in the yml configuration. That should be fixed now.

Status: Fixed » Closed (fixed)

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