search

ResponsiveImagesCG / wp-tevko-responsive-images

Fully responsive image plugin for wordpress
449 stars 53 forks source link

Cleanup tests #247

Closed jaspermdegroot closed 8 years ago

jaspermdegroot commented 8 years ago

I cherry-picked the commit from the 'fix-deprecation-warnings' branch (PR #246) to avoid merge conflicts. So we should merge that PR first and then rebase this 'cleanup-tests' branch to drop the commit before merging this PR.

I don't really understand how the @group numbering works. Some tests belong to a group, some don't. @joemcgill maybe you can give that look or explain it to me.

joemcgill commented 8 years ago

Commit 549a61c from #246 (now af96fd5) looks good to me. Either merge the former and drop the cherry-picked version or close #246 and use the commit in this branch.

In b8bf33d, I see where using $sizes would be confusing, but we should be consistent about using either $sources or $srcset.

The @group attribute on the tests is a way of running only specific sets of tests, by running a command like phpunit --group 226. I sometimes mark certain tests with the related ticket number for reference.

jaspermdegroot commented 8 years ago

Commit 549a61c from #246 (now af96fd5) looks good to me. Either merge the former and drop the cherry-picked version or close #246 and use the commit in this branch.

I merged #246. The commit has been dropped here after rebase.

In b8bf33d, I see where using $sizes would be confusing, but we should be consistent about using either $sources or $srcset.

Ok. I used $sources when it's a foreach loop ($sources as $source). I changed it to $srcset.

I sometimes mark certain tests with the related ticket number for reference.

Now I understand the number and why not all belong to a group.

joemcgill commented 8 years ago

Looks good. I assume you'll rebase before merging? Either way, you have my :+1: