Re: [Gallery3] Unit test updates
Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> Fri, 21 Jun 2013 08:06:48 +0200
| Newsgroups | gmane.comp.web.gallery.devel |
|---|---|
| Message-ID | <CA+z51A4mXRgCpaQYBRRjOEzTLgKfPEyRvvyPA821ODHBA_gxeA@mail.gmail.com> |
--===============6263437938300849924== Content-Type: multipart/alternative; boundary=001a11c2e4de6f012304dfa3df6f --001a11c2e4de6f012304dfa3df6f Content-Type: text/plain; charset=UTF-8 I tried playing with this, and now remember why we put it in Controller::execute() - if we put it before the initial request generation, we (unsurprisingly) don't have access to a populated Request object. In particular, this screws up Theme::load_themes(), which uses Request to figure out if we're in admin mode or have an override. It also screws up our check for a robot user_agent - while it doesn't bomb the code, Request::user_agent() checks against a still-unset value. A few options: - hack Theme::load_themes() and the robot check to no longer need Request. While technically possible, this sounds messy... I don't think it's a good approach. - move a couple things out of the "ready" event, and leave them in Controller::execute(). This gets the job done without major hacks, but the fact that we need to do this in the first place hints that a better approach would be to... - have two events, one right before the initial Request and one right after. This is my current favorite approach. Next question: if we do the two-event option, what should they be called? Some possibilities: - Before request: init (exact Kohana analog), routes (most common application), bootstrap, before_initial_request - After request: gallery_ready (current name), ready (shorter name), after_initial_request Thoughts? Shad On 21 June 2013 01:12, Bharat Mediratta <[email protected]> wrote: > > I think that's reasonable. I've never been 100% happy with putting the > "ready" event in Controller::execute. > > > On Thu, Jun 20, 2013 at 3:44 PM, Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote: > >> I like the idea of reducing the init.php's down to zero and putting it in >> an event, and agree that trying to keep track of this isn't a good idea. >> In fact, we could probably just move the execution of the already-existing >> "ready" event from Controller::execute() to either bootstrap.php or >> index.php (just before the initial request generation). I wonder - should >> it be renamed "init" to make its analog more obvious? >> >> Then, we just add a Route override that clears out the Route list, and >> use it in Module to activate/deactivate things as well as in the unit test. >> >> Thoughts? >> >> Take care, >> Shad >> >> On 21 June 2013 00:12, Bharat Mediratta <[email protected]> wrote: >> >>> >>> I've been wondering about that as well, but I wasn't thinking about it >>> from the unit test perspective. I was thinking more along the lines of >>> activating and deactivating modules during a request's lifetime. Eg, if we >>> deactivate and reactivate a module several times, what is a developer >>> supposed to do about their code in init.php? Should devs guard against >>> their code being called multiple times? It feels messy to me. >>> >>> I think a better approach would be to minimize what goes into init.php, >>> preferably down to zero. We can leave the facility around, but since we're >>> only using it for routes I think we could easily have an event hook which >>> sets routes, then we can call that from Module whenever we add a module. >>> >>> If we want to be smart about removing routes when a module is >>> deactivated (not essential for us to do right now, IMO) we could always >>> track which routes were added when we call the hook and remove them when >>> the module goes away, similar to how we do Graphics::deactivate_rules >>> and BlockManager::deactivate_blocks... >>> >>> >>> >>> >>> On Thu, Jun 20, 2013 at 10:52 AM, Shad Laws <shad-xpYdmXCiSuZWk0Htik3J/[email protected]> wrote: >>> >>>> Hey Bharat, >>>> >>>> It looks like, except for the REST-related unit tests I'm still >>>> wrapping up, the only tests left are controller auth and XSS - woohoo! >>>> >>>> I did, however, hit a minor snag. I got everything running well and >>>> passing, hit a reset button, and then everything broke with the REST tests >>>> - crap! After a bit of digging, I found the problem: the REST tests won't >>>> pass on a fresh installation. They only pass if you activate the rest >>>> module, *then* run unit tests. >>>> >>>> The issue is the rest module's init.php file. Like the other init.php >>>> files (e.g. in tag), it's designed to run *before* the bootstrap routes are >>>> defined. That way, they get precedence. However, if you roll a fresh >>>> install (which has rest disabled) straight into a unit test, it bombs - the >>>> bootstrap runs first, then rest's init.php later... and it doesn't work :-/ >>>> >>>> One thought I have is to override the Route class so we can clear them >>>> and restart from scratch, then make the unit tests re-load them all... but >>>> that sounds kinda hacky and flaky (e.g. what happens if an init.php does >>>> something besides load routes?). Any better thoughts? >>>> >>>> Take care, >>>> Shad >>>> >>> >>> >> > --001a11c2e4de6f012304dfa3df6f Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable I tried playing with this, and now remember why we put it in Controller::ex= ecute() - if we put it before the initial request generation, we (unsurpris= ingly) don't have access to a populated Request object. =C2=A0In partic= ular, this screws up Theme::load_themes(), which uses Request to figure out= if we're in admin mode or have an override. =C2=A0It also screws up ou= r check for a robot user_agent - while it doesn't bomb the code, Reques= t::user_agent() checks against a still-unset value.<div> <br></div><div>A few options:</div><div>- hack Theme::load_themes() and the= robot check to no longer need Request. =C2=A0While technically possible, t= his sounds messy... I don't think it's a good approach.</div><div>-= move a couple things out of the "ready" event, and leave them in= Controller::execute(). =C2=A0This gets the job done without major hacks, b= ut the fact that we need to do this in the first place hints that a better = approach would be to...</div> <div>- have two events, one right before the initial Request and one right = after. =C2=A0This is my current favorite approach.</div><div><br></div><div= >Next question: if we do the two-event option, what should they be called? = =C2=A0Some possibilities:</div> <div>- Before request: init (exact Kohana analog), routes (most common appl= ication), bootstrap, before_initial_request</div><div>- After request: gall= ery_ready (current name), ready (shorter name), after_initial_request</div> <div><div><br></div><div>Thoughts?</div><div>Shad</div><div><br><div class= =3D"gmail_quote">On 21 June 2013 01:12, Bharat Mediratta <span dir=3D"ltr">= <<a href=3D"mailto:[email protected]" target=3D"_blank">bharat@menalto.= com</a>></span> wrote:<br> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex"><div dir=3D"ltr"><br><div>I think that's= reasonable. =C2=A0I've never been 100% happy with putting the "re= ady" event in Controller::execute.</div> </div><div class=3D"gmail_extra"><br><br><div class=3D"gmail_quote"> On Thu, Jun 20, 2013 at 3:44 PM, Shad Laws <span dir=3D"ltr"><<a href=3D= "mailto:shad-xpYdmXCiSuZWk0Htik3J/[email protected]" target=3D"_blank">shad-xpYdmXCiSuZWk0Htik3J/[email protected]</a>></spa= n> wrote:<br><blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;b= order-left:1px #ccc solid;padding-left:1ex"> I like the idea of reducing the init.php's down to zero and putting it = in an event, and agree that trying to keep track of this isn't a good i= dea. =C2=A0In fact, we could probably just move the execution of the alread= y-existing "ready" event from Controller::execute() to either boo= tstrap.php or index.php (just before the initial request generation). =C2= =A0I wonder - should it be renamed "init" to make its analog more= obvious?<div> <br></div><div>Then, we just add a Route override that clears out the Route= list, and use it in Module to activate/deactivate things as well as in the= unit test.</div><div><br></div><div>Thoughts?</div><div><br></div><div> Take care,</div><div>Shad</div><div><div><div><div><div><br><div class=3D"g= mail_quote">On 21 June 2013 00:12, Bharat Mediratta <span dir=3D"ltr"><<= a href=3D"mailto:[email protected]" target=3D"_blank">[email protected]</= a>></span> wrote:<br> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex"><div dir=3D"ltr"><br><div>I've been wond= ering about that as well, but I wasn't thinking about it from the unit = test perspective. =C2=A0I was thinking more along the lines of activating a= nd deactivating modules during a request's lifetime. =C2=A0Eg, if we de= activate and reactivate a module several times, what is a developer suppose= d to do about their code in init.php? =C2=A0Should devs guard against their= code being called multiple times? =C2=A0It feels messy to me.</div> <div><br></div><div>I think a better approach would be to minimize what goe= s into init.php, preferably down to zero. =C2=A0We can leave the facility a= round, but since we're only using it for routes I think we could easily= have an event hook which sets routes, then we can call that from Module wh= enever we add a module.</div> <div><br></div><div>If we want to be smart about removing routes when a mod= ule is deactivated (not essential for us to do right now, IMO) we could alw= ays track which routes were added when we call the hook and remove them whe= n the module goes away, similar to how we do=C2=A0Graphics::deactivate_rule= s and=C2=A0BlockManager::deactivate_blocks...</div> <div><br></div><div><br></div></div><div class=3D"gmail_extra"><br><br><div= class=3D"gmail_quote">On Thu, Jun 20, 2013 at 10:52 AM, Shad Laws <span di= r=3D"ltr"><<a href=3D"mailto:shad-xpYdmXCiSuZWk0Htik3J/[email protected]" target=3D"_blank">shad@s= hadlaws.com</a>></span> wrote:<br> <blockquote class=3D"gmail_quote" style=3D"margin:0 0 0 .8ex;border-left:1p= x #ccc solid;padding-left:1ex">Hey Bharat,<div><br></div><div>It looks like= , except for the REST-related unit tests I'm still wrapping up, the onl= y tests left are controller auth and XSS - woohoo!</div> <div><br></div><div>I did, however, hit a minor snag. =C2=A0I got everythin= g running well and passing, hit a reset button, and then everything broke w= ith the REST tests - crap! =C2=A0After a bit of digging, I found the proble= m: the REST tests won't pass on a fresh installation. =C2=A0They only p= ass if you activate the rest module, *then* run unit tests.</div> <div><br></div><div>The issue is the rest module's init.php file. =C2= =A0Like the other init.php files (e.g. in tag), it's designed to run *b= efore* the bootstrap routes are defined. =C2=A0That way, they get precedenc= e. =C2=A0However, if you roll a fresh install (which has rest disabled) str= aight into a unit test, it bombs - the bootstrap runs first, then rest'= s init.php later... and it doesn't work :-/</div> <div><br></div><div>One thought I have is to override the Route class so we= can clear them and restart from scratch, then make the unit tests re-load = them all... but that sounds kinda hacky and flaky (e.g. what happens if an = init.php does something besides load routes?). =C2=A0Any better thoughts?</= div> <div><br></div><div>Take care,</div><div>Shad</div> </blockquote></div><br></div> </blockquote></div><br></div></div></div> </div></div></blockquote></div><br></div> </blockquote></div><br></div></div> --001a11c2e4de6f012304dfa3df6f-- --===============6263437938300849924== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline ------------------------------------------------------------------------------ This SF.net email is sponsored by Windows: Build for Windows Store. http://p.sf.net/sfu/windows-dev2dev --===============6263437938300849924== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline __[ g a l l e r y - d e v e l ]_________________________ [ list info/archive --> http://gallery.sf.net/lists.php ] [ gallery info/FAQ/download --> http://gallery.sf.net ] --===============6263437938300849924==--