New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Basic tests for "Add tests" issue #3 #16

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
3 participants
@doyleyoung

doyleyoung commented Apr 28, 2015

The tests are broken down into subtests which group the major errors, actions and subroutines into common units.

There are a number of commented tests that could be fixed to hit edge cases and increase the branch and conditional coverage.

Devel::Cover reports the following:

File stmt bran cond sub pod time total
lib/App/ModuleBuildTiny.pm 68.3 52.2 34.2 100.0 12.5 100.0 62.2
Total 68.3 52.2 34.2 100.0 12.5 100.0 62.2

These tests only cover the basics, but should be a good start for issue #3.

Basic tests for "Add tests" issue #3
The tests are broken down into subtests which group the major errors, actions and subroutines into common units.

There are a number of commented tests that could be fixed to hit edge cases and increase the branch and conditional coverage.

Devel::Cover reports the following:
---------------------------- ------ ------ ------ ------ ------ ------ ------
File                           stmt   bran   cond    sub    pod   time  total
---------------------------- ------ ------ ------ ------ ------ ------ ------
lib/App/ModuleBuildTiny.pm     68.3   52.2   34.2  100.0   12.5  100.0   62.2
Total                          68.3   52.2   34.2  100.0   12.5  100.0   62.2
---------------------------- ------ ------ ------ ------ ------ ------ ------

These tests only cover the basics, but should be a good start for issue #3.
@karenetheridge

This comment has been minimized.

Show comment
Hide comment
@karenetheridge

karenetheridge May 1, 2015

Don't forget use strict; use warnings;! :)

karenetheridge commented on t/AppModuleBuildTiny.t in 1e1b8fb May 1, 2015

Don't forget use strict; use warnings;! :)

@karenetheridge

This comment has been minimized.

Show comment
Hide comment
@karenetheridge

karenetheridge May 1, 2015

This shouldn't be needed -- and when running via make test or prove -b, it is not a good idea as we want blib to be used for the module source, not lib.

karenetheridge commented on t/AppModuleBuildTiny.t in 1e1b8fb May 1, 2015

This shouldn't be needed -- and when running via make test or prove -b, it is not a good idea as we want blib to be used for the module source, not lib.

@karenetheridge

This comment has been minimized.

Show comment
Hide comment
@karenetheridge

karenetheridge May 1, 2015

Generally it's better to test the exception that is thrown, rather than just "it died" -- it might be dying for a different reason than you think it is, masking other issues.

karenetheridge commented on t/AppModuleBuildTiny.t in 1e1b8fb May 1, 2015

Generally it's better to test the exception that is thrown, rather than just "it died" -- it might be dying for a different reason than you think it is, masking other issues.

@karenetheridge

This comment has been minimized.

Show comment
Hide comment
@karenetheridge

karenetheridge May 1, 2015

These is() tests can all be collapsed down to one with is_deeply, or Test::Deep::cmp_deeply (you're already loading Test::Deep).

karenetheridge commented on t/AppModuleBuildTiny.t in 1e1b8fb May 1, 2015

These is() tests can all be collapsed down to one with is_deeply, or Test::Deep::cmp_deeply (you're already loading Test::Deep).

@Leont

This comment has been minimized.

Show comment
Hide comment
@Leont

Leont May 1, 2015

Owner

It's a nice start, but there are significant issues with it:

In the big picture, a lot of things are much more testable when actually interacting with the filesystem in a tempdir. A lot of these tests don't truly test the effects of the operations, only side-effects.

  • use_ok is not really useful in this case. In the unlikely event App::ModuleBuildTiny doesn't load, you don't just want that first test to fail, you want the whole testrun to die. There's no point in continuing after that point.
  • dies on bad action looks ok, though I'm not sure it makes sense to add a dependency on Test::Exception just for the dies_ok function. Might as wel do is(eval { ...; 1}, undef, "Description").
  • uptodate handles missing destination: This is just a stub.
  • valid actions: the return values of mbtiny aren't meaningful currently, only it (not) throwing exceptions is. The results on the filesystem are what matters.
  • prereqs_for: looks ok to me
  • get_files This isn't really testing anything, doing work on a tempdir would be useful here.
  • uptodate: this isn't testing anything yet
  • find: looks ok to me
  • mbt_version: cute trick. Better than nothing, but I think I'd still prefer to do this for real instead of emulated.
  • get_meta Testing the default meta contents is not particularly useful. CPAN::Meta does the heavy lifting and is well tested. Actually, it's rather fragile too the way it's set up (if the A::MBT version changes, the test breaks). The merge test would need a regenerate => [ 'META.json' ] to actually DWIM. Doing this in a tempdir really sounds like a better idea. Also there's no advantage to using Test::Deep over Test::More's is_deeply when used in this way.
Owner

Leont commented May 1, 2015

It's a nice start, but there are significant issues with it:

In the big picture, a lot of things are much more testable when actually interacting with the filesystem in a tempdir. A lot of these tests don't truly test the effects of the operations, only side-effects.

  • use_ok is not really useful in this case. In the unlikely event App::ModuleBuildTiny doesn't load, you don't just want that first test to fail, you want the whole testrun to die. There's no point in continuing after that point.
  • dies on bad action looks ok, though I'm not sure it makes sense to add a dependency on Test::Exception just for the dies_ok function. Might as wel do is(eval { ...; 1}, undef, "Description").
  • uptodate handles missing destination: This is just a stub.
  • valid actions: the return values of mbtiny aren't meaningful currently, only it (not) throwing exceptions is. The results on the filesystem are what matters.
  • prereqs_for: looks ok to me
  • get_files This isn't really testing anything, doing work on a tempdir would be useful here.
  • uptodate: this isn't testing anything yet
  • find: looks ok to me
  • mbt_version: cute trick. Better than nothing, but I think I'd still prefer to do this for real instead of emulated.
  • get_meta Testing the default meta contents is not particularly useful. CPAN::Meta does the heavy lifting and is well tested. Actually, it's rather fragile too the way it's set up (if the A::MBT version changes, the test breaks). The merge test would need a regenerate => [ 'META.json' ] to actually DWIM. Doing this in a tempdir really sounds like a better idea. Also there's no advantage to using Test::Deep over Test::More's is_deeply when used in this way.
@doyleyoung

This comment has been minimized.

Show comment
Hide comment
@doyleyoung

doyleyoung May 12, 2015

Thank you for your reviews, I made updates to address each point.

Devel::Cover now reports the following:

File stmt bran cond sub pod time total
lib/App/ModuleBuildTiny.pm 75.4 59.0 42.1 100.0 12.5 100.0 68.5
Total 75.4 59.0 42.1 100.0 12.5 100.0 68.5

doyleyoung commented May 12, 2015

Thank you for your reviews, I made updates to address each point.

Devel::Cover now reports the following:

File stmt bran cond sub pod time total
lib/App/ModuleBuildTiny.pm 75.4 59.0 42.1 100.0 12.5 100.0 68.5
Total 75.4 59.0 42.1 100.0 12.5 100.0 68.5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment