Hi guys, I''ve been seeing lots of commits lately without tests, for instance: http://github.com/rails/rails/commit/b6e56efe07cb3c2e999216f995403aa9206226a2 http://github.com/rails/rails/commit/238a6bb62dc153743a0abc6eb1e35392ac799d65 http://github.com/rails/rails/commit/1dab1d380377f1a2a60da43bc22989d55632d246 http://github.com/rails/rails/commit/5c63be1f92edcd3ed60fae90b8eb129da19c5099 Most of these commits appear to fix some sort of regression, but no tests are added to make sure we can catch such a regression in the future. Is this done on purpose? Manfred --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
Hi, I''ve also been worried about the lack of testing present in some commits lately. ActionPack, and especially the Rack related code, seem to lack lots of necessary testing. Afaik the Rails policy used to be; have good test coverage or don''t commit. Has this been changed? Eloy On 18 feb 2009, at 09:52, Manfred Stienstra wrote:> > Hi guys, > > I''ve been seeing lots of commits lately without tests, for instance: > > http://github.com/rails/rails/commit/b6e56efe07cb3c2e999216f995403aa9206226a2 > http://github.com/rails/rails/commit/238a6bb62dc153743a0abc6eb1e35392ac799d65 > http://github.com/rails/rails/commit/1dab1d380377f1a2a60da43bc22989d55632d246 > http://github.com/rails/rails/commit/5c63be1f92edcd3ed60fae90b8eb129da19c5099 > > Most of these commits appear to fix some sort of regression, but no > tests are added to make sure we can catch such a regression in the > future. Is this done on purpose? > > Manfred > > >--~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Tue, Feb 24, 2009 at 1:19 PM, Eloy Duran <eloy.de.enige@gmail.com> wrote:> I''ve also been worried about the lack of testing present in some > commits lately. ActionPack, and especially the Rack related code, seem > to lack lots of necessary testing. > Afaik the Rails policy used to be; have good test coverage or don''t > commit. Has this been changed?Many of those changes are related to development reloading which is really hard to test with traditional unit tests. This has been really frustrating on my part because I have to manually run though some stupid checklist for development reloading vs production for every patch I accept related to this. Seriously, if you have any good ideas how to automate real functional tests in this area I''d love to get them in. -- Joshua Peek --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Feb 24, 2009, at 8:36 PM, Joshua Peek wrote:> > On Tue, Feb 24, 2009 at 1:19 PM, Eloy Duran > <eloy.de.enige@gmail.com> wrote: >> I''ve also been worried about the lack of testing present in some >> commits lately. ActionPack, and especially the Rack related code, >> seem >> to lack lots of necessary testing. >> Afaik the Rails policy used to be; have good test coverage or don''t >> commit. Has this been changed? > > Many of those changes are related to development reloading which is > really hard to test with traditional unit tests. This has been really > frustrating on my part because I have to manually run though some > stupid checklist for development reloading vs production for every > patch I accept related to this. > > Seriously, if you have any good ideas how to automate real functional > tests in this area I''d love to get them in.My first intuition is to spawn new Rails process from a test and see if it behaves the way it should. I''ll try to free up some time tomorrow to give it a try. Manfred --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
That sounds like a plan. Thanks for the response. Eloy On 24 feb 2009, at 20:41, Manfred Stienstra wrote:> > > On Feb 24, 2009, at 8:36 PM, Joshua Peek wrote: > >> >> On Tue, Feb 24, 2009 at 1:19 PM, Eloy Duran >> <eloy.de.enige@gmail.com> wrote: >>> I''ve also been worried about the lack of testing present in some >>> commits lately. ActionPack, and especially the Rack related code, >>> seem >>> to lack lots of necessary testing. >>> Afaik the Rails policy used to be; have good test coverage or don''t >>> commit. Has this been changed? >> >> Many of those changes are related to development reloading which is >> really hard to test with traditional unit tests. This has been really >> frustrating on my part because I have to manually run though some >> stupid checklist for development reloading vs production for every >> patch I accept related to this. >> >> Seriously, if you have any good ideas how to automate real functional >> tests in this area I''d love to get them in. > > My first intuition is to spawn new Rails process from a test and see > if it behaves the way it should. I''ll try to free up some time > tomorrow to give it a try. > > Manfred > > >--~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Tue, Feb 24, 2009 at 12:41 PM, Manfred Stienstra <manfred@gmail.com> wrote:> My first intuition is to spawn new Rails process from a test and see > if it behaves the way it should. I''ll try to free up some time > tomorrow to give it a try.The tricky part of this would be to ensure that the spawned environment is identical to the current environment. For example, environment/rake variables, ruby interpreter being used, etc, etc. -- Chad --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Wed, Feb 25, 2009 at 3:03 PM, Chad Woolley <thewoolleyman@gmail.com> wrote:> The tricky part of this would be to ensure that the spawned > environment is identical to the current environment. For example, > environment/rake variables, ruby interpreter being used, etc, etc.That sounds like a job for forking. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
> That sounds like a job for forking.That makes accumulating the failures from the child process essentially impossible though, you''d end up with crazy output intertwined with the parent process. -- Cheers Koz --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Wed, Feb 25, 2009 at 4:59 PM, Michael Koziarski <michael@koziarski.com> wrote:> >> That sounds like a job for forking. > > That makes accumulating the failures from the child process > essentially impossible though, you''d end up with crazy output > intertwined with the parent process.My assumption was that we would create a pipe before forking, each of the two processes would close one end of the pipe and talk into the other, and pass back the output/status. The parent would check that it matches (assert_equal "everything worked" line_read_from_pipe). But I think forking doesn''t really work on Windows (haven''t tried, but it''s certainly hard to implement for other languages so I imagine it doesn''t work in Ruby either). But, this is all a problem with spawning in general, so if we do go with spawning/forking as suggested, testing on Windows would get quite difficult. Anyway, I''m just saying that forking would be better than spawning in general. As for whether this is the ''right'' way to test Rack integration etc. - I''m not sure we can solve the problem all neatly wrapped up inside the test harness, personally. We might just have to deal with it at a higher level - having multiple continuous integration targets, one with each different thing we need to test against. We don''t have to run the full test suite for each, and many could live on the same server with a bit of coaxing. --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
>>> That sounds like a job for forking. >> >> That makes accumulating the failures from the child process >> essentially impossible though, you''d end up with crazy output >> intertwined with the parent process.Well, let''s just talk code shall we (: I''ll write up some stuff and present it to the mailinglist, we can debate whether it''s a good solution afterwards. About support for testing on Windows: any help is appreciated, I don''t understand Windows. Manfred --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
Ok, I spent some time digging through commits and writing tests. The tests I wrote actually don''t have anything to do with reloading because I couldn''t find a recent commit that required any forking or spawning to test. http://github.com/Manfred/rails/commits/railties-tests Josh, can you point me to a specific piece of code that would require such a test? Manfred On Feb 25, 8:15 am, Manfred Stienstra <manf...@gmail.com> wrote:> >>> That sounds like a job for forking. > > >> That makes accumulating the failures from the child process > >> essentially impossible though, you''d end up with crazy output > >> intertwined with the parent process. > > Well, let''s just talk code shall we (: I''ll write up some stuff and > present it to the mailinglist, we can debate whether it''s a good > solution afterwards. About support for testing on Windows: any help is > appreciated, I don''t understand Windows. > > Manfred--~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
On Wed, Feb 25, 2009 at 4:25 AM, Manfred Stienstra <manfred@gmail.com> wrote:> > Ok, I spent some time digging through commits and writing tests. The > tests I wrote actually don''t have anything to do with reloading > because I couldn''t find a recent commit that required any forking or > spawning to test. > > http://github.com/Manfred/rails/commits/railties-testsThose look nice (didn''t run ''em myself). Mocks to the rescue! Good job, thanks. -- Chad --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---
> Ok, I spent some time digging through commits and writing tests. The > tests I wrote actually don''t have anything to do with reloading > because I couldn''t find a recent commit that required any forking or > spawning to test. > > http://github.com/Manfred/rails/commits/railties-tests > > Josh, can you point me to a specific piece of code that would require > such a test?Any of the changes like 69c049f5ab45bf9bfb0d269acea0773581905fd4 That fixed problems with storing AR objects in the session: 1) the session stores call marshall.dump 2) and the marshalling code checks respond_to? 3) and the dispatcher hook had triggered reload! 4) So the class that the instance refers to has been nuked (c.f. earlier discussions) 5) So the session serialization fails Great work so far, if you can figure out a reliable way to do regression testing with reloaded code, then I''ll gladly (gladly!) apply it :)> Manfred > > On Feb 25, 8:15 am, Manfred Stienstra <manf...@gmail.com> wrote: >> >>> That sounds like a job for forking. >> >> >> That makes accumulating the failures from the child process >> >> essentially impossible though, you''d end up with crazy output >> >> intertwined with the parent process. >> >> Well, let''s just talk code shall we (: I''ll write up some stuff and >> present it to the mailinglist, we can debate whether it''s a good >> solution afterwards. About support for testing on Windows: any help is >> appreciated, I don''t understand Windows. >> >> Manfred > > >-- Cheers Koz --~--~---------~--~----~------------~-------~--~----~ You received this message because you are subscribed to the Google Groups "Ruby on Rails: Core" group. To post to this group, send email to rubyonrails-core@googlegroups.com To unsubscribe from this group, send email to rubyonrails-core+unsubscribe@googlegroups.com For more options, visit this group at http://groups.google.com/group/rubyonrails-core?hl=en -~----------~----~----~----~------~----~------~--~---