Code review comment for lp://staging/~stevanr/linaro-license-protection/phpunit-tests

Revision history for this message
Stevan Radaković (stevanr) wrote :

> > > > 1) Please revert your changes in 'testing/test_click_through_license.py'
> > > back.
> > > > 2) Add instructions how to install PHP_CodeCoverage, I had to follow
> > > following
> > > > steps to get running your test:
> > > > sudo apt-get remove phpunit
> > > > sudo apt-get upgrade pear
> > > > sudo pear channel-discover pear.phpunit.de
> > > > sudo pear channel-discover pear.symfony-project.com
> > > > sudo pear channel-discover components.ez.no
> > > > sudo pear update-channels
> > > > sudo pear upgrade-all
> > > > sudo pear install --alldeps phpunit/PHPUnit
> > > > sudo apt-get install phpunit
> > > > 3) You have incorrect expected value for file.with.more.dots, should be:
> > > > $this->assertEquals("file", $filename_array[0]);
> > > > $this->assertEquals("with.more.dots", $filename_array[1]);
> > >
> > > 2) I was able to do the tests on fresh 12.04 only by installing phpunit
> > > package. Maybe we should discuss this.
> >
> > Anyway lets add information about if the execution of tests fails with
> > PHP_CodeCoverage error
> >
> > > 3) The test actually shows that the code should be refactored since the
> > > extension of 'file.with.more.dots' would be 'dots' and not
> 'with.more.dots'.
> >
> > How about *.tar.bz2?
>
> PHP function pathinfo returns only the last part of the filename with multiple
> dots, i.e. in example you provided it will return only bz2. Since it's a
> standard PHP function my opinion is that we should copy it's behavior.

I've just realized that this function is not used at all in license.php, and basically there are some standard library functions in PHP that already do this task so I think we should be OK if we remove this method as part of the refactoring. What do you think?

« Back to merge proposal