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

Check to prevent Perl version mismatch in reports #25

Merged
merged 3 commits into from Apr 23, 2016

Conversation

Projects
None yet
2 participants
@mnlagrasta
Contributor

mnlagrasta commented Aug 5, 2015

Added a version check to prevent the submission of a report where the Perl version provided by the build.log entry does not match the Perl version which is being used to generate the report. The goal of this is to reduce invalid reports that can easily occur while using a more advanced Perl deployment scheme such as PerlBrew or plenv. I've also added a command line parameter to bypass this check has been added. Tests have been added and updated as needed.

Unfortunately, I had to modify most of the existing tests to pass in the --ignore-version flag since the versions in the test build.log files would almost never match the current Perl version. I considered updating the tests to create their data files on demand but decided the more minimal modifications were best for now.

This may be a solution for CPAN RT bug 102995 https://rt.cpan.org/Public/Bug/Display.html?id=102995 . However, it is unclear to me if that bug is implying a deeper issue in version detection in the case of PerlBrew/plenv.

mnlagrasta added some commits Aug 5, 2015

Added a version check to prevent the submission of a report where the…
… Perl version provided by the build.log entry does not match the Perl version which is being used to generate the report. The goal of this is to reduce invalid reports that can easily occur while using a more advanced Perl deployment scheme such as PerlBrew or plenv. I've also added a command line parameter to bypass this check has been added. Tests have been added and updated as needed.
@mnlagrasta

This comment has been minimized.

Show comment
Hide comment
@mnlagrasta

mnlagrasta Aug 5, 2015

Contributor

Merging in kwalitee fix from #24 to prevent travis build failures.

Contributor

mnlagrasta commented Aug 5, 2015

Merging in kwalitee fix from #24 to prevent travis build failures.

@mnlagrasta

This comment has been minimized.

Show comment
Hide comment
@mnlagrasta

mnlagrasta Aug 5, 2015

Contributor

Sorry to muddy things with a PR that fails the travis build. The local "make test" worked fine. I'll check it out and update when I can. Not sure of the best etiquette for this.

Contributor

mnlagrasta commented Aug 5, 2015

Sorry to muddy things with a PR that fails the travis build. The local "make test" worked fine. I'll check it out and update when I can. Not sure of the best etiquette for this.

@garu

This comment has been minimized.

Show comment
Hide comment
@garu

garu Aug 6, 2015

Owner

@mnlagrasta Hey Mike! Thanks for the PR, and thanks for taking the time to analyze this. The "non-zero exit status" on several tests does indeed look weird :(

Owner

garu commented Aug 6, 2015

@mnlagrasta Hey Mike! Thanks for the PR, and thanks for taking the time to analyze this. The "non-zero exit status" on several tests does indeed look weird :(

@mnlagrasta

This comment has been minimized.

Show comment
Hide comment
@mnlagrasta

mnlagrasta Aug 6, 2015

Contributor

Looks fine now. I used the wrong hash key '-' vs '_' in the tests. Not sure how that got by me on my local tests.

Contributor

mnlagrasta commented Aug 6, 2015

Looks fine now. I used the wrong hash key '-' vs '_' in the tests. Not sure how that got by me on my local tests.

@garu garu merged commit fe17d21 into garu:master Apr 23, 2016

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on master at 90.323%
Details
@garu

This comment has been minimized.

Show comment
Hide comment
@garu

garu Apr 23, 2016

Owner

Works perfectly, @mnlagrasta! Thanks!

Owner

garu commented Apr 23, 2016

Works perfectly, @mnlagrasta! Thanks!

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