Skip to content
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

Move the test aggregator to BEGIN so that it gets preload friendly #4

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

miyagawa
Copy link

@miyagawa miyagawa commented Nov 9, 2012

This moves the test attribute collector to the BEGIN phase, so that even if Test::Class is loaded with eval() or after the fork, it should run fine.

All tests pass, and my sample test code that makes use of Test::Class runs fine as well. But I wouldn't be surprised if this breaks some obscure .t file with bunch of inline classes in it.

This patch is made per popular requests to make Test::Class forkprove friendly. I myself don't use Test::Class at all, but didn't want to add a special code to deal with Test::Class :)
miyagawa/forkprove#7

Review on Reviewable

@miyagawa
Copy link
Author

miyagawa commented Nov 9, 2012

Guess we would have to update the documentation if this patch can be accepted, since require $some_test_class without BEGIN now works fine with the patch :)

I also tested a couple of CPAN modules with Test::Class - Poet, CHI, Reaction, Data::Model - all tests pass fine.

@karenetheridge
Copy link
Contributor

Is this still good/still desired?

@Ovid
Copy link
Collaborator

Ovid commented Nov 17, 2014

I would say that it's definitely desired, but I would recommend a dev release first since it's a behavioral change. Test::Class has never been as popular as it should be, so I wouldn't be surprised if there's an impact, but if there's a long enough lead time on the dev release, it should forestall complaints.

(Edit: no, it won't forestall complaints if it breaks someone's code, but it will at least be a very reasonable defense).

@karenetheridge
Copy link
Contributor

corollary: "this change needs a champion" :)

@miyagawa
Copy link
Author

Idea: find dists on CPAN with cpangrep that uses Test::Class and smoke them with this change?

@rafl
Copy link

rafl commented Nov 18, 2014

I've just release Test-Class-0.48-TRIAL to CPAN with these changes applied on top of the latest master branch, as well as with a small documentation update. I'm currently smoking various Test::Class users to see if I can observe any change in behaviour.

The code of the 0.48-TRIAL release is available at https://github.com/rafl/test-class.

@rafl
Copy link

rafl commented Nov 18, 2014

Turns out miyagawa's fix only works on perl version 5.12.0 and later. On older versions the attribute handler won't receive the typeglob for the symbol it's being called for, which prevents Test::Class from working.

https://gist.github.com/rafl/6fd2c8f0e669105d17ad shows an example of this.

I've uploaded Test-Class-0.49-TRIAL.tar.gz addressing this with a documentation change.

@karenetheridge
Copy link
Contributor

I don't see the commits at https://github.com/rafl/test-class/commits/master

  • did you push?

On Tue, Nov 18, 2014 at 6:40 AM, Florian Ragwitz [email protected]
wrote:

I've just release Test-Class-0.48-TRIAL to CPAN with these changes applied
on top of the latest master branch, as well as with a small documentation
update. I'm currently smoking various Test::Class users to see if I can
observe any change in behaviour.

The code of the 0.48-TRIAL release is available at
https://github.com/rafl/test-class.


Reply to this email directly or view it on GitHub
#4 (comment).

@karenetheridge
Copy link
Contributor

It also appears that the changes in the stable 0.48 were lost -
https://metacpan.org/diff/file?target=FLORA/Test-Class-0.49-TRIAL/&source=ETHER/Test-Class-0.48/

On Tue, Nov 18, 2014 at 9:47 AM, Karen Etheridge [email protected] wrote:

I don't see the commits at
https://github.com/rafl/test-class/commits/master - did you push?

On Tue, Nov 18, 2014 at 6:40 AM, Florian Ragwitz <[email protected]

wrote:

I've just release Test-Class-0.48-TRIAL to CPAN with these changes
applied on top of the latest master branch, as well as with a small
documentation update. I'm currently smoking various Test::Class users to
see if I can observe any change in behaviour.

The code of the 0.48-TRIAL release is available at
https://github.com/rafl/test-class.


Reply to this email directly or view it on GitHub
#4 (comment).

@karenetheridge
Copy link
Contributor

It also appears that the changes in the stable 0.48 were lost -
https://metacpan.org/diff/file?target=FLORA/Test-Class-0.49-TRIAL/&source=ETHER/Test-Class-0.48/

Ah, that one is my fault - I didn't push the 0.48 release! (will do so
when I can recover from my hard drive crash...)

On Tue, Nov 18, 2014 at 9:51 AM, Karen Etheridge [email protected] wrote:

It also appears that the changes in the stable 0.48 were lost -
https://metacpan.org/diff/file?target=FLORA/Test-Class-0.49-TRIAL/&source=ETHER/Test-Class-0.48/

On Tue, Nov 18, 2014 at 9:47 AM, Karen Etheridge [email protected] wrote:

I don't see the commits at
https://github.com/rafl/test-class/commits/master - did you push?

On Tue, Nov 18, 2014 at 6:40 AM, Florian Ragwitz <
[email protected]> wrote:

I've just release Test-Class-0.48-TRIAL to CPAN with these changes
applied on top of the latest master branch, as well as with a small
documentation update. I'm currently smoking various Test::Class users to
see if I can observe any change in behaviour.

The code of the 0.48-TRIAL release is available at
https://github.com/rafl/test-class.


Reply to this email directly or view it on GitHub
#4 (comment).

@miyagawa
Copy link
Author

It's pushed to the preload branch and there are tags for 0.49 as well
https://github.com/rafl/test-class/tree/preload

@karenetheridge
Copy link
Contributor

I am looking to merge this back into the main master stream as 0.50-stable, but it looks like tests are still failing pre-5.12, which isn't good: http://matrix.cpantesters.org/?dist=Test-Class%200.49-TRIAL

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants