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

Already on GitHub? Sign in to your account

First stab at porting Test::Reporter functionality from cpantimes to cpanminus #209

Open
wants to merge 4 commits into
from

Conversation

Projects
None yet
3 participants

tobyink commented Mar 3, 2013

A couple of issues that probably need input from @miyagawa before merge...

  • Currently if everything is in place (Test::Reporter, etc installed; and a metabase profile has been configured) it defaults to sending reports. Should there be an explicit opt-in, rather than an opt-out?
  • It doesn't output any mention of the reports to STDOUT. This is because the reports are sent between outputting Building and testing $dist ... and outputting OK, so there's nowhere good to output it. I can play around with this, but it may require splitting the Building and testing line into two lines.

@tobyink tobyink referenced this pull request Mar 3, 2013

Open

CPAN Testers reporting #184

Owner

miyagawa commented Mar 3, 2013

There definitely needs to be an option to opt-in or out, but am not sure which should be the default :)

Owner

miyagawa commented Mar 3, 2013

It makes sense to split Building and testing, at least when test reporting is in place.

tobyink commented Mar 3, 2013

There is currently a --skip-report opt-out.

For test reporting to work, they need to install Test::Reporter, etc, plus set up a metabase profile (which would be hard to do accidentally) so I've kinda been treating that as an implicit opt-in.

tobyink commented Mar 4, 2013

Oh yes, another issue to think about:

  • It expects the metabase profile details to be in ~/.cpantesters/metabase_id.json. This is where CPAN.pm expects it, and also (I think) cpanplus. But it's not a hard requirement - we could keep it somewhere in the .cpanm directory instead.
Owner

miyagawa commented Mar 4, 2013

I think it makes sense to share the location, unless there's any specific reason not to.

It's understandable but arguable whether setting up the metabase profile could be taken as a sign to opting in testers. I know that it's hard to set it up right now, but it could be easily changed to make it much easier (hopefully :D).

I will think about it.

tobyink commented Apr 14, 2013

There have been quite a few changes to cpanm lately and this patch is drifting further and further from being cleanly applicable. Any chance it can be applied soonish?

Owner

miyagawa commented Apr 14, 2013

Talk to garu, since he believes this patch isn't in a good shape.

You're right that this can't be easily applicable.

Contributor

garu commented Apr 15, 2013

Hey guys! Sorry for being a bit late joining the conversation. I've talked to each of you individually, but it's best if I document it here too :-)

As part of the work in the last two QA Hackathons, I've worked with David Golden, Barbie and Andreas to extract the metabase populating bits from CPAN::Reporter into a common reporter client that could be shared by all cpan clients out there, namely, CPAN::Testers::Common::Client.

https://metacpan.org/module/CPAN::Testers::Common::Client
https://github.com/garu/CPAN-Testers-Common-Client

My main issue with this patch is that a Metabase report isn't just an email - or at least it won't be for very long now that we got Metabase facts being collected - and even as it is, a lot of extra data needs to be properly gathered and formatted for the test report to be as accurate and thorough as possible, not to mention properly parsed by CPAN Testers' scripts. There are a lot of potential variations in both test results and environments in which cpanm runs, so the current implementation would benefit a lot if integrated to the Common::Client.

I co-maint CPAN::Reporter with David, and BingOS already agreed on using the common client on cpanp as soon as it gets out of core (5.18). I have also finally released the 'proof of concept' tool that I was working on with Miyagawa during last year's QA Hackathon:

https://metacpan.org/module/GARU/App-cpanminus-reporter-0.01/bin/cpanm-reporter

it works fairly well, and you can see its very first web output here, courtesy of Barbie:

http://www.cpantesters.org/cpan/report/212e65ee-a50a-11e2-b4af-b7694470b1b1

Like I said, there's a lot going on under the hood there as well, and now that we have the first Metabase facts being collected by the common client, Barbie and Andreas are looking forward to being able to take CPAN Testers to a whole new level - as long as test reporting clients send that data to the servers.

That being said, I like @tobyink's approach and, even though cpanm-reporter works on its own, if we can create something that, if installed, cpanm will use to generate and send the report, then even better. Might be a new patch giving CPAN::Testers::Common::Client its required data and sending things through Test::Reporter, might be an adaptation of App::cpanminus::reporter turned into a "plugin" and making the cpanm patch something very lightweight and simple like:

eval { require App::cpanminus::reporter; App::cpanminus::reporter->new($self)->send) };

And it could be something else entirely (as long as it uses the Common::Client).

Oh, btw: I'll gladly give you guys co-maint on App::cpanminus::reporter if you want to fiddle with this idea. Just let me know :-)

Cheers!

Owner

miyagawa commented Apr 15, 2013

Thanks garu!

Random thoughts:

  • I like both doing the from-inside approach (tobyink's) and from-outside approach (garu's). Guess doing it inside can make more accurate data, but needs a little tight connection (hence needs to be merged sooner than later). Outside approach makes cpanm a blackbox, but so long as it can access the previous build directory and build log output, it might be fine.
  • I'm happy to accept patches to make the outside-approach more reliable, including but not limited to: output the last build directory name in the output so that you don't need to rely on latest-build symlinks or timestamps, redirect output to build.log even when verbose option is on, dump more build option data in a separate file, etc.
Contributor

garu commented Apr 15, 2013

Sounds great! Meanwhile, I'll make a few changes to Toby's patch just so it relies on the common client and as such can give all the required data to the CPAN Testers database in the required fashion. I'm leaving Lancaster today and it's a long trip back to Rio (with tons of catching up to do at work), but hopefully I'll have a patch for you guys this weekend.

Cheers!

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