Paul Cochrane

Paul Cochrane

Multi-purpose software developer and geek

Helping to clean CPAN: March 2016

7 minute read

… where I get out my virtual broom and sweep up cruft in my assigned distribution for this month’s edition of the CPAN Pull Request Challenge.

This month’s module: MojoX::Transaction::WebSocket76

MojoX::Transaction::WebSocket76 is a container for WebSocket transactions as described in the hixie-76 draft.

First impressions

In this section I try to get a feeling for the state of the module, how up to date it is, how often people are contributing to it, how many other distributions are depending on it, how many bugs/issues it currently has, what the CPANTS kwalitee is, etc.

This seems to be sort of a niche module and hasn’t been updated in a while, the main issue seems to be that it doesn’t work with current Mojolicious versions. Other than that, there are some cosmetic fixups one could undertake.

Initial inspection of the source code

After forking the repo and cloning a local copy, I have a look at the project to see what build system it uses, if the test suite works and the tests pass, if it could do with a Travis-CI config file (or if present, if it can be updated). The initial inspection often gives inspiration for the first batch of pull requests.

  • ExtUtils::MakeMaker project
  • convert the README to markdown?
  • perl Makefile.PL
  • cpanm --installdeps . necessary for dependencies
  • no .travis.yml
  • also doesn’t use coveralls
  • according to perlver, the minimum Perl version is 5.10.1, however this is explicit only in Makefile.PL and the rest is implicitly 5.8.0. This is actually due to the Mojolicious dependency: Mojolicious depends upon

    = 5.10.1

  • make test fails
t/00-use.t ........... 1/1 Bailout called.  Further testing stopped:

#   Failed test 'use MojoX::Transaction::WebSocket76;'
#   at t/00-use.t line 10.
#     Tried to use 'MojoX::Transaction::WebSocket76'.
#     Error:  Undefined subroutine &Mojo::Transaction::WebSocket::DEBUG called at /home/cochrane/Projekte/OSSProjekte/mojox-transaction-websocket76/blib/lib/MojoX/Transaction/WebSocket76.pm line 11, <DATA> line 2125.
# BEGIN failed--compilation aborted at /home/cochrane/Projekte/OSSProjekte/mojox-transaction-websocket76/blib/lib/MojoX/Transaction/WebSocket76.pm line 11, <DATA> line 2125.
# Compilation failed in require at t/00-use.t line 10, <DATA> line 2125.
# BEGIN failed--compilation aborted at t/00-use.t line 10, <DATA> line 2125.
Use of uninitialized value $MojoX::Transaction::WebSocket76::VERSION in concatenation (.) or string at t/00-use.t line 13, <DATA> line 2125.
# Testing MojoX::Transaction::WebSocket76 , Perl 5.018004, /home/cochrane/perl5/perlbrew/perls/perl-5.18.4/bin/perl
# Looks like you failed 1 test of 1.
FAILED--Further testing stopped.
Makefile:849: recipe for target 'test_dynamic' failed
make: *** [test_dynamic] Error 1
  • it turns out that Mojo::Transaction::WebSocket removed DEBUG etc. in version 6+ of Mojolicious
  • with Mojolicious <= 5.17, the tests pass
  • … which means that a requirement is cpanm Mojolicious~"<=5.17"
  • need TEST_POD=1 in order to test pod coverage. Gives 3 naked subroutines:
t/02-pod-coverage.t .. 1/1
#   Failed test 'Pod coverage on MojoX::Transaction::WebSocket76'
#   at /home/cochrane/perl5/perlbrew/perls/perl-5.18.4/lib/site_perl/5.18.4/Test/Pod/Coverage.pm
line 133.
# Coverage for MojoX::Transaction::WebSocket76 is 0.0%, with 3 naked
subroutines:
#       build_frame
#       parse_frame
#       server_handshake
# Looks like you failed 1 test of 1.
  • need RELEASE_TESTING=1 to check the MANIFEST; need Test::CheckManifest in order to run this test
  • after T::CM is installed, the test complains about a lot of files which could be skipped. -> add to MANIFEST.SKIP
  • convert hard tabs to spaces? -> consistently indented with tabs, no need to convert as it looks like the author has this style
  • need to add a .gitignore

Code Coverage

Looking at the code coverage can give an indication of code quality. If the project is well covered, this means most changes made in pull requests can be made with some confidence that any problems will be caught by the test suite. If the code coverage is low, then this is something that one could address as a pull request (or set of pull requests).

In EUMM and Build::Module projects, one simply needs to install Devel::Cover and run

$ cover -test

In Dist::Zilla projects, one needs to install the Dist::Zilla::App::Command::cover plugin, after which the code coverage can be checked via:

$ dzil cover

In this distribution, the coverage is:

$ cover -test
23.8% statement coverage; 22.3% total coverage

… which is unfortunately somewhat low. One could look into increasing test coverage.

POD checks

The utility podchecker searches through Perl source code for POD which might not conform to the POD standard, and thus not necessarily be parseable by all POD parsers. Fixing any issues found by podchecker has the positive effect of also removing any warnings noted in the project’s documentation displayed on MetaCPAN.

Running podchecker gives the following errors and warnings:

$ find ./lib -name '*.pm' | xargs podchecker |& grep ERROR # 0 errors
$ find ./lib -name '*.pm' | xargs podchecker |& grep WARN # 0 warnings

So nothing which needs fixing, yay!

Nit-picking

Check for trailing whitespace

Some projects consider this a must, and will disallow commits to be submitted which contain trailing whitespace (the Linux kernel is an example project where trailing whitespace isn’t permitted). Other projects see whitespace cleanup as simply nit-picking. Either way one sees it personally, this could be a useful pull request to a project, so it’s worthwhile fixing and submitting; the worst that can happen is that the pull request is closed unmerged.

To look for files with trailing whitespace, run git grep ' $'. It can be helpful to load the files found directly into vim:

$ vim $(git grep ' $' | cut -d':' -f1 | sort | uniq)

No files need to be fixed.

perlcritic

Perl::Critic will show up many potential issues for Perl code. By simply running perlcritic on the lib and t directories, one can get a further handle on the code’s quality.

$ perlcritic lib
  • code before strictures warnings
$ perlcritic t
  • expression form of eval used

Stale URLs

Links to websites can go out of date, so it’s a good idea to see if they need updating or removing. A quick grep finds all the links. After which, we just need to see which links need fixing, if any.

$ git grep 'http://\|https://\|ftp://\|git://'

They all look ok.

Spell check POD

Good documentation can be a wonder to read. Not everyone’s docs are awesome, however we can keep the error rate to a minimum. A quick spell check will pick up most typos that don’t need to be there and fixing them can help improve the quality of a project.

In general, we want to find all files containing POD and run a spell checker (e.g. aspell) over all files, fixing typos we come across as we go. Not all projects require this much effort, however here’s a general-ish way to look for and check all POD in a project:

$ files_with_pod=$(find ./lib -name '*.pm' -o -name '*.pod' \
                 | xargs podchecker 2>&1 \
                 | grep 'pod syntax' | cut -d' ' -f1)
$ for filename in $files_with_pod
do
    pod2text $filename > $filename.txt;
    aspell -c $filename.txt;
done

Now look for .bak file and check differences between it and the output .txt file, the process looks roughly like this:

$ find ./ -name '*.bak'
$ diff -u lib/Path/To/Module.pm.pm.txt.bak lib/Path/To/Module.pm.txt

Then update the appropriate .pm and/or .pod files as necessary.

Things look good.

GitHub issues

  • 1 to look into
    • it mentions that one can’t use the module with Mojolicious 6.40 and later. This is because some constants from Mojolicious, needed in MojoX::Transaction::WebSocket76, have been removed.
    • a fix for this issue was submitted in PR#12.

Kwalitee tests

Although CPANTS is the main kwalitee reference, one can also run the kwalitee tests locally. One can use the t/kwalitee.t test script from http://peateasea.de/cpan-pull-request-challenge/ for this purpose. However, the script only uses Test::Kwalitee which doesn’t cover as many metrics as CPANTS. Test::Kwalitee::Extra uses set of metrics closer to that used on CPANTS, so replace the kwalitee_ok call with simply use Test::Kwalitee::Extra. More information about the many options to Test::Kwalitee::Extra can be found on the MetaCPAN page.

Run the kwalitee tests in an ExtUtils::MakeMaker or Build::Module distribution like so:

$ RELEASE_TESTING=1 prove t/99kwalitee.t

or, if the distribution uses Dist::Zilla, run

$ dzil test --author --release

Core and extras kwalitee tests pass.

Overview of the pull requests made

Conclusion

Many thanks to DIONYS for merging my pull requests!

The author reacted quickly to pull requests, which was really nice. A new version of the dist was released to CPAN on the 16th of April 2016, yay!

Support

If you liked this post and want to see more like this, please buy me a coffee!

buy me a coffee logo

You may also enjoy

Self nerd-sniping with macaroni cheese

19 minute read

The other day I baked macaroni cheese in an oval-shaped oven dish and ended up going down quite the rabbit hole when wanting to divide it into eight equal po...

Avoid git push --tags

5 minute read

Today I learned that there are better ways to push Git tags to an upstream repository than the bad habit I’d gotten into, i.e.: git push --tags. It turns ou...