Problem/Motivation

2 of the existing 3 tests in \Drupal\Tests\file\Kernel\Plugin\migrate\source\d7\FileTest does not test the 'filepath'.

Proposed resolution

Add 'filepath' to the expected data for tests #2 and #3.

Remaining tasks

Patch, review, commit.

CommentFileSizeAuthor
#4 Screen Shot 2020-07-18 at 9.42.38 PM.png322.02 KBtanubansal
#4 Screen Shot 2020-07-18 at 9.42.34 PM.png322.16 KBtanubansal
#4 Screen Shot 2020-07-18 at 9.39.27 PM.png340.32 KBtanubansal
#2 3159849-2.patch865 bytesquietone

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented
StatusFileSize
new 3159849-2.patch865 bytes

Simple.

quietone’s picture

quietone commented
Status: Active » Needs review
tanubansal’s picture

tanubansal commented
StatusFileSize
new Screen Shot 2020-07-18 at 9.39.27 PM.png340.32 KB
new Screen Shot 2020-07-18 at 9.42.34 PM.png322.16 KB
new Screen Shot 2020-07-18 at 9.42.38 PM.png322.02 KB

After adding latest patch, tested Filetest.php.
Issue is resolved and can be moved to RTBC

quietone’s picture

quietone commented

@tanubansal, Hi and welcome to Drupal!

Thank you for contributing to the review of this patch! You can save yourself some time and not upload screen shots of the changes. (I hate to think what that would look like for a large patch). Mostly folks use the browser extension dreditor to review and discuss code. It makes the job much easier. When you save you tested Filtetest.php does that mean you ran the test using phpunit?
Oh, there is also a wealth of information on drupal.org on how we contribute, there is a section for Reviewing Patches. Apologies if you have already seen that.

Cheers!

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Category: Bug report » Task
Priority: Normal » Minor
Status: Needs review » Reviewed & tested by the community

Straightforward, adding additional assertions to the tests and they still pass is a good thing. The d6 equivalent test already tests for filepath. Easy
+1 to RTBC

Status: Reviewed & tested by the community » Needs work

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

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
commented
Status: Needs work » Reviewed & tested by the community
alexpott’s picture

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

Committed and pushed 07c913e241 to 9.1.x and 2859ded499 to 9.0.x and 5840b83261 to 8.9.x. Thanks!

Backported to 8.9.x as this is a test only change.

@tanubansal thank you for looking into this issue.

Posting screenshots of your codebase or CLI does not advance the issue, since the automated testing infrastructure tells us whether the patch applies correctly.

So, I've removed the issue credit for that screenshot. In the future, you can get credit for issues by reading the issue to understand its purpose, and posting your review or testing of that purpose. Thank you!

For example @mikelutz includes the helpful information that the D6 test already tests this and so receives credit as this is helpful.

  • alexpott committed 07c913e on 9.1.x 
    Issue #3159849 by quietone, mikelutz: Add missing tests of filepath to...

  • alexpott committed 2859ded on 9.0.x 
    Issue #3159849 by quietone, mikelutz: Add missing tests of filepath to...

  • alexpott committed 5840b83 on 8.9.x 
    Issue #3159849 by quietone, mikelutz: Add missing tests of filepath to...

Status: Fixed » Closed (fixed)

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