Rails Ruby on Rails → 3.0.6

Comments and changes to this ticket

• 

  Rohit Arondekar October 6th, 2010 @ 07:16 AM

  • State changed from “new” to “open”
  • Milestone cleared.
  • Importance changed from “” to “Low”

  No need for the .gitignore files actually.

  ~/code९ mkdir google
  ~/code९ ls google -a
  .  ..
  ~/code९ rails new google -p
    .
    .
    .
  ~/code९ tree google
  google
  |-- config
  |-- log
  `-- tmp
  
  3 directories, 0 files
  

  Can you write a patch (with tests) for this? This should help: http://rails.lighthouseapp.com/projects/8994/sending-patches

• 

  Aditya Sanghi October 6th, 2010 @ 07:51 AM

  @Rohit

  Not sure if i understood your comment correctly, but i would imagine, that in -p pretend mode, not just .gitignore but NO directories should be created. Yes?

  @tilo, if you are interested in writing a patch, awesome, if not, i don't mind going ahead and doing it. I need the practice :)

• 

  Rohit Arondekar October 6th, 2010 @ 08:25 AM

  @Aditya

  The OP mentioned .gitignore file and other things.

  let's assume the XYZ Rails project folder already exists, e.g. it contains old git repo with older Rails on a different branch

  mkdir XYZ ls -a XYZ . .. .git .gitignore

  However you don't need all that to reproduce this issue. That's what I was pointing out. :)

• 

  Aditya Sanghi October 6th, 2010 @ 09:28 AM

  From my initial investigation it seems like the problem happens when the generator action has a combination of empty_directory "config" followed by a inside "config" block call.

      def log
        empty_directory "log"
  
        inside "log" do
          %w( server production development test ).each do |file|
            create_file "#{file}.log"
            chmod "#{file}.log", 0666, :verbose => false
          end
        end
      end
  

  I'll try to investigate if this is indeed the right way to write the action and its a thor bug or we should be writing the action differently.
  The app test should also be fixed to ensure that it fails properly, but i've detected that location already.

• 

  Rohit Arondekar October 6th, 2010 @ 09:29 AM

  I just investigated a little bit and it's possible that this is a Thor issue. Although I'm not sure, do give a look into Thor in case you can't find the bug in Rails source.

• 

  Rohit Arondekar October 6th, 2010 @ 09:40 AM

  Aditya, I think you've found the problem. In Thor — lib/thor/actions.rb

      def inside(dir='', config={}, &block)
        verbose = config.fetch(:verbose, false)
  
        say_status :inside, dir, verbose
        shell.padding += 1 if verbose
        @destination_stack.push File.expand_path(dir, destination_root)
  
  -->      FileUtils.mkdir_p(destination_root) unless File.exist?(destination_root)
        FileUtils.cd(destination_root) { block.arity == 1 ? yield(destination_root) : yield }
  
        @destination_stack.pop
        shell.padding -= 1 if verbose
      end
  

  The line with --> is creating the directory as it's not checking for pretend. If you do check for pretend by appending || options[:pretend] then the next line fails (obviously) as there is no dir.

• 

  Aditya Sanghi October 6th, 2010 @ 09:49 AM

  @rohit yes i now believe it should be a thor bug. In the pretend mode, i believe the "inside" method does not respect the pretend flag.

      def inside(dir='', config={}, &block)
        verbose = config.fetch(:verbose, false)
  
        say_status :inside, dir, verbose
        shell.padding += 1 if verbose
        @destination_stack.push File.expand_path(dir, destination_root)
  
        FileUtils.mkdir_p(destination_root) unless File.exist?(destination_root) # <--- what about pretend??
        FileUtils.cd(destination_root) { block.arity == 1 ? yield(destination_root) : yield }
  
        @destination_stack.pop
        shell.padding -= 1 if verbose
      end
  

  I suppose the next step would be to wait for a Thor response (i've created a bug http://github.com/wycats/thor/issues/issue/74 there), but this shouldn't stop us from checking in a failing test meanwhile? Rails CI would start popping up with failures right? What the general rule around this?

• 

  Aditya Sanghi October 6th, 2010 @ 09:51 AM

  i should check my email and refresh before posting. we're literally working on the "same lines". :)

• 

  Rohit Arondekar October 6th, 2010 @ 10:04 AM

  Aditya, yeah me too! :P

  Ok good news is that it's definitely a Thor bug. Confirmed this with Jose. However he is too busy to work on this. Can you try and write the patch for Thor? The main thing to keep in mind is that in the pretend mode, the messages like —

        create  config/locales
        create  config/locales/en.yml
        create  config/boot.rb
        create  config/database.yml
  

  Should still appear. So Jose has suggested that in pretend mode, you create the directory, pretend to create the files and then remove the directory.

  In any case append the above information regarding the inside method to the Thor ticket and continue discussing the issue there. I'll leave this ticket open till the issue is resolved in Thor.

  Also I think you can go ahead with the failing test for Rails too. :)

• 

  Aditya Sanghi October 6th, 2010 @ 01:38 PM

  Alrighty, thor patch sent with tests (http://github.com/wycats/thor/pull/75)
  

  Working on the failing test. Seem like assert_no_file is acting real fishy!

• 

  David Trasbo October 6th, 2010 @ 04:32 PM

  • State changed from “open” to “resolved”

  IMHO, there's no need to duplicate tests across projects. This was a Thor issue (fixed now) and its regression test lives in Thor.

  Marking this resolved as the fix has been merged into Thor.

• 

  Aditya Sanghi October 6th, 2010 @ 04:39 PM

  David,

  I agree with you completely but since i'm kinda new to this, i started investigating if there was already a test in Rails to test the --pretend functionality or not.

  I found that there is.

    def test_application_generate_pretend
      run_generator ["testapp", "--pretend"]
      DEFAULT_APP_FILES.each{ |path| assert_no_file path }
    end
  

  Had a look at DEFAULT_APP_FILES and wondered why it wasn't failing.
  Discovered it didnt list "config" and "tmp". Fair enough, now we really dont need to test for that since we'll kinda make up for it in thor.
  BUT, it DID list "log" and "log" was being created and left around, so WHY was this test "assert_no_file path" passing?

  I think this test is broken. It was testing for the wrong files in the wrong place, passing and thus giving a false positive.

  It needs to be to really work

    def test_application_generate_pretend
      run_generator ["testapp", "--pretend"]
      DEFAULT_APP_FILES.each{ |path| assert_no_file File.join("testapp",path) }
    end
  

• 

  David Trasbo October 6th, 2010 @ 05:23 PM

  • State changed from “resolved” to “open”

  Aditya,

  Okay, we definitely want neither duplicate nor broken tests. :)

  Re-opening this.

• 

  Aditya Sanghi October 6th, 2010 @ 06:38 PM

  and here is the failing test for rails patch. Applying this will make the rails tests fail correctly until the next release of thor.

  • prepend_test.diff 933 Bytes
• 

  David Trasbo October 6th, 2010 @ 06:41 PM

  I don't get it. assert_no_file uses destination_root just like assert_file, so it shouldn't be necessary to use File.join. I think it would be desirable to get to the root of this issue and make assert_no_file work properly.

• 

  Aditya Sanghi October 6th, 2010 @ 07:07 PM

    def test_invalid_application_name_is_fixed
      run_generator [File.join(destination_root, "things-43")]
      assert_file "things-43/config/environment.rb", /Things43::Application\.initialize!/
      assert_file "things-43/config/application.rb", /^module Things43$/
    end
  

  There are other examples too, but i think if a parameter is provided to run_generator, then it must also be prefixed before checking for the file.

  In the test case i fixed, "testapp" is being provided as a parameter, but the test happens without it. Thus it needs to be fixed. I see other example in app_generator_test.rb which all assert_file with app name prefixed.

• 

  Aditya Sanghi October 8th, 2010 @ 01:02 PM

  • Assigned user set to “Rohit Arondekar”
• 

  Rohit Arondekar October 9th, 2010 @ 01:35 AM

  • Assigned user changed from “Rohit Arondekar” to “José Valim”
• 

  José Valim October 9th, 2010 @ 10:03 AM

  David, even if the current test was working, adding new tests is always a good choice. Remember that, in the future, we decide to not use thor anymore, we need to have a reliable and consistent test suite in Rails.

  Aditya, I am still pondering what you said that pretend should not do anything (as some filesystem may not even have permissions for operation). That said, I believe that the correct fix in Thor is to simply not create the directory.

  If should be something like this:

      if options[:pretend]
        yield(destination_root)
      else
        FileUtils.cd(destination_root) { yield(destination_root) }
      end
  

  Do you think you can try a new thor patch and tell us if the result still work?

• 

  Aditya Sanghi October 9th, 2010 @ 10:17 AM

  Sure José, I'll try your suggestion out in thor and send a pull request there like last time.

• 

  Jeremy Kemper October 15th, 2010 @ 11:02 PM

  • Milestone set to “3.0.2”

  [bulk edit]

• 

  Ryan Bigg October 16th, 2010 @ 02:28 AM

  • Tag cleared.

  Automatic cleanup of spam.

• 

  Aditya Sanghi November 1st, 2010 @ 11:12 AM

  Jose,

  You've now fixed thor not to generate stuff in the pretend mode. We should based on your comments above also add the tests to rails when we make rails depend on the latest version of thor.

• 

  Ryan Bigg November 8th, 2010 @ 01:49 AM

  Automatic cleanup of spam.

• 

  José Valim November 11th, 2010 @ 04:25 PM

  @Aditya Sanghi Yes and Rails already depends on latest thor. That said, do you think you can provide a test case? Thanks!

• 

  Aditya Sanghi November 12th, 2010 @ 07:00 AM

  @jose, i've added the test in the patch above already.

  There was a test for pretend mode already but it was giving a false positive. I've fixed that in the patch attached to this ticket.

  Are you referring to some other test that i've not understood, perhaps?

• 

  José Valim November 12th, 2010 @ 07:06 AM

  Sorry, I have missed this patch as well. I will apply it later! Thanks!

• 

  Santiago Pastorino November 15th, 2010 @ 09:04 PM

  • Milestone cleared.

  [bulk edit]

• 

  Aditya Sanghi December 22nd, 2010 @ 04:57 PM

  Gentle Bump! Perhaps push this to master before its forgotten?

• 

  Santiago Pastorino February 12th, 2011 @ 09:58 PM

  • Milestone set to “3.0.5”

  [bulk edit]

• 

  Santiago Pastorino February 27th, 2011 @ 03:15 AM

  • Milestone changed from “3.0.5” to “3.0.6”

  [bulk edit]

• 

  Repository March 23rd, 2011 @ 05:12 PM

  • State changed from “open” to “resolved”

  (from [3a7c7dc73d6111e7c821a81d9c56024dd35769a5]) Fix test for prepend giving a false positive. [#5716 state:resolved]

  Signed-off-by: José Valim jose.valim@gmail.com
  https://github.com/rails/rails/commit/3a7c7dc73d6111e7c821a81d9c560...