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

Test Reports #11

Merged
merged 1 commit into from Feb 12, 2018

Conversation

Projects
None yet
2 participants
@genio
Contributor

genio commented Aug 10, 2017

Hi,

I received this dist in this month's Pull Request Challenge. It seems like a pretty clean dist with not a lot of fixes to jump into other than some minor testing setup changes. Overall, this is a pretty trivial change that should hopefully make testing a bit easier and provide reports from users and smoke testers when something blows up.

No actual code changes were made.

A few of the test files were moved into a more common xt/ location. To run those tests, it's easy to now just prove -lr xt. Those tests have had their eval-wrapped use statements removed to be just plain use statements instead as these tests will not be run by anyone but the author.

A new test file, 00-report-prereqs.t now exists in the root of the t/ directory to show a good amount of info during the test which helps debug when an error appears from CPAN Testers reports or from users.

The .gitignore file was altered a bit to ensure no files get tracked after perl Makefile.PL.

The MANIFEST was altered to indicate the xt tests described above.

The Makefile.PL file was altered to show any and all requirements, even the trivial ones. This just shows a more-complete prereqs report on a test run. A few creative greps over the code and tests were made to ensure we have them all listed. The prereqs within the xt/ directory are not listed as there isn't a spec for DEVELOP_REQUIRES in the Makefile.PL and we're not tracking prereqs with a cpanfile.

A few of the test files were moved into a more common xt/ location.
To run those tests, it's easy to now just ```prove -lr xt```. Those
tests have had their eval-wrapped use statements removed to be just
plain use statements instead as these tests will not be run by any-
one but the author.

A new test file, 00-report-prereqs.t now exists in the root of the
t/ directory to show a good amount of info during the test which
helps debug when an error appears from CPAN Testers reports or
from users.

The .gitignore file was altered a bit to ensure no files get tracked
after ```perl Makefile.PL```.

The MANIFEST was altered to indicate the xt tests described above.

The Makefile.PL file was altered to show any and all requirements,
even the trivial ones. This just shows a more-complete prereqs
report on a test run. A few creative greps over the code and tests
were made to ensure we have them all listed. The prereqs within
the xt/ directory are not listed as there isn't a spec for
DEVELOP_REQUIRES in the Makefile.PL and we're not tracking prereqs
with a cpanfile.

No actual code changes were made.

Overall, this is a pretty trivial change that should hopefully make
testing a bit easier and provide reports from users and smoke testers
when something blows up.
@manwar

This comment has been minimized.

Show comment
Hide comment
@manwar

manwar Aug 15, 2017

Owner

Hi @genio

Thanks for your contribution, much appreciated :-)
I have the following points to share with you with regard to your pull request.

  1. I liked the idea having dependencies explicitly noted in Makefile.PL script. (well done).
  2. I liked the addition of test "00-report-prereqs.t". (well done).
  3. I am not a big fan of "xt/" personally. However I am not against its use either. However if we could find a way to streamlined like "make test", that would have make life so easy.
  4. Also I am not a big fan of ".gitignore" either personally. I don't like hiding un-wanted files and folders. I want it to be on my face and force me to clean up after build, e.g. "make realclean".

Please don't get me wrong, I appreciate your effort and time. However I am still not fully convinced about (3 and 4).
Can you please share your views on the same?
I prefer discussions rather than forcing my views.

Best Regards,
Mohammad S Anwar

Owner

manwar commented Aug 15, 2017

Hi @genio

Thanks for your contribution, much appreciated :-)
I have the following points to share with you with regard to your pull request.

  1. I liked the idea having dependencies explicitly noted in Makefile.PL script. (well done).
  2. I liked the addition of test "00-report-prereqs.t". (well done).
  3. I am not a big fan of "xt/" personally. However I am not against its use either. However if we could find a way to streamlined like "make test", that would have make life so easy.
  4. Also I am not a big fan of ".gitignore" either personally. I don't like hiding un-wanted files and folders. I want it to be on my face and force me to clean up after build, e.g. "make realclean".

Please don't get me wrong, I appreciate your effort and time. However I am still not fully convinced about (3 and 4).
Can you please share your views on the same?
I prefer discussions rather than forcing my views.

Best Regards,
Mohammad S Anwar

@manwar manwar merged commit b940a18 into manwar:master Feb 12, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

manwar added a commit that referenced this pull request Feb 12, 2018

@manwar

This comment has been minimized.

Show comment
Hide comment
@manwar

manwar Feb 12, 2018

Owner

Thanks @genio

Owner

manwar commented Feb 12, 2018

Thanks @genio

@genio

This comment has been minimized.

Show comment
Hide comment
@genio

genio Feb 12, 2018

Contributor

Oh, wow. Sorry about that @manwar. I never saw your reply to this PR.

On number 3: I'm more a fan of hiding release and author tests off into xt/ that way we don't have to add things like skip unless $ENV{'TEST_POD'} and whatnot. I certainly get that it's easier on non Dist::Zilla dists to leave them in t/ so you can turn the testing on and off with environment variables, but the clear delineation is overall better in my view with xt. I tend towards this after having dealt more and more with DZil, so my viewpoint may be skewed.

On number 4: .gitignore solely helps to not check in new files errantly. This is not of a particular large issue with me, but some devs I've worked with tend to do something akin to:

git add .
git commit -m 'updated foo bar'

Such a process doesn't catch me because I tend to commit and comment specifically on what I want to check in via git commit foo.bar -m 'updated foo bar'. However, preventing people from committing things that shouldn't be in the repo is easily enough done with the .gitignore that I'm of the "why not?" opinion. No real strong feelings on that topic either.

Again, sorry I didn't respond to your questions. I either completely missed the notification (likely) or didn't get it somehow.

Hope all's well!

Contributor

genio commented Feb 12, 2018

Oh, wow. Sorry about that @manwar. I never saw your reply to this PR.

On number 3: I'm more a fan of hiding release and author tests off into xt/ that way we don't have to add things like skip unless $ENV{'TEST_POD'} and whatnot. I certainly get that it's easier on non Dist::Zilla dists to leave them in t/ so you can turn the testing on and off with environment variables, but the clear delineation is overall better in my view with xt. I tend towards this after having dealt more and more with DZil, so my viewpoint may be skewed.

On number 4: .gitignore solely helps to not check in new files errantly. This is not of a particular large issue with me, but some devs I've worked with tend to do something akin to:

git add .
git commit -m 'updated foo bar'

Such a process doesn't catch me because I tend to commit and comment specifically on what I want to check in via git commit foo.bar -m 'updated foo bar'. However, preventing people from committing things that shouldn't be in the repo is easily enough done with the .gitignore that I'm of the "why not?" opinion. No real strong feelings on that topic either.

Again, sorry I didn't respond to your questions. I either completely missed the notification (likely) or didn't get it somehow.

Hope all's well!

@genio genio deleted the genio:genio/prc-build branch Feb 12, 2018

@manwar

This comment has been minimized.

Show comment
Hide comment
@manwar

manwar Feb 13, 2018

Owner

No worries, @genio :-)
I appreciate your effort in responding in details.

Best Regards,
Mohammad S Anwar

Owner

manwar commented Feb 13, 2018

No worries, @genio :-)
I appreciate your effort in responding in details.

Best Regards,
Mohammad S Anwar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment