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

Use require to load support files instead of load #1043

Closed
roberts1000 opened this issue Oct 3, 2016 · 31 comments
Closed

Use require to load support files instead of load #1043

roberts1000 opened this issue Oct 3, 2016 · 31 comments
Assignees
Labels
🧷 pinned Tells Stalebot not to close this issue

Comments

@roberts1000
Copy link

I've run into a situation where I would like to control the load order for files in the features/support folder.

I got the impression, from reading The Cucumber Book, that Cucumber would behave similar to RSpec and only autoload features/support/. I tried moving the files into features/setup/ and manually requiring the files from env.rb. I quickly discovered that Cucumber wants to take over and load everything, regardless of the sub folder. (To be precise, RSpec doesn't autoload files in spec/support unless it's specifically configured to do so.)

Next I tried moving all the files back into the features/support folder, but having File A explicitly require File B t to make sure File B is loaded before File A is executed. Unfortunately, this doesn't work either because Cucumber uses the load method to load the support files. Is there any reason Cucumber couldn't use require instead of load in this method?

https://github.com/cucumber/cucumber-ruby/blob/master/lib/cucumber/rb_support/rb_language.rb#L107

That would give us much more flexibility in controlling the order of how support files are loaded. require would guarantee the file is only loaded once. It wouldn't matter if Cucumber got to it first or I manually required it somewhere.

As it stands now, my only options are to combine the two dependent files into a single file, OR, move files outside of the feature folders; neither of which are good options. The use of load blocks any attempt to establish order because it will blindly load the file even if it has already been loaded.

@mattwynne
Copy link
Member

I don't believe there's a good reason for using load, no. Probably just an accident of history. I'll give it some thought.

I'm not near the code but do all the tests pass if you change it?

@roberts1000
Copy link
Author

roberts1000 commented Oct 3, 2016

Thanks for thinking about it. It looks like there are about 70 failures.

@mattwynne
Copy link
Member

Ah interesting. The test failures seem to mostly be down to this which allows all Cucumber's own tests to be run in the same Ruby process. This is for performance - it's much slower to run the test suite when we work with files and processes for each scenario.

So Cucumber itself might work fine, but our tests are relying on the use of load, I think. We could build a new strategy for the Aruba process that uses fork which would be faster than the spawning strategy, but that might be too much of a yak-shave.

What do you think @roberts1000? Are you keen to dig any deeper?

@roberts1000
Copy link
Author

So you're saying there's a chance.... :)

I wish I could contribute more to the discussion at this point, but this the first time I've looked at the internals of Cucumber. I'm interested in digging deeper. I'll need to carve out some time to get up to speed on Cucumber development.

@emmakun
Copy link

emmakun commented Oct 27, 2016

@roberts1000 have you tried using require_relative instead of require?
I have a custom World class in a file under my support folder and then a helpers.rb file inside the same folder.
Custom world depends on helpers, so in my world file I have this:

require_relative 'helpers'

class CustomWorld
  include Helpers
  # other code
end

And it works just fine.

Now, if what you need is to have a file loaded first, then you can simply use require_relative from env.rb itself, for instance, I have a base_page.rb file where I have defined my Page class, that all my other pages inherit from, but I didn't want to have to require it in all the page files, so in my env file I just did this:

require_relative 'pages/base_page'

And now it is loaded before all the other support files, so I can use its contents from anywhere.

@roberts1000
Copy link
Author

roberts1000 commented Oct 28, 2016

We can use require or require_relative to control the load order ... to an extent. You're correct that it will ensure your file is loaded before you use it. The problem is that Cucumber uses the load method internally which is not going to play nice with require or require_relative. I suspect if you add some put statements in the helpers.rb, you'll find that it's being loaded twice. This can cause problems.

require and require_relative are smart enough to not load a file twice. load always loads. Also, they don't play nice together. If require fires first, load will come along and load it again. If load fires first, require... will load it again.

If Cucumber used require internally, we could definitely control the load order with require or require_relative.

@azohra
Copy link

azohra commented Oct 28, 2016

@alvaroemmanuel it is not the order which is the issue, it is the fact that it will load a file more than once and thus execute any dangling code multiple times.

@mattwynne
Copy link
Member

Thinking about this some more, I wonder if we could have two, configurable strategies, so the user can choose whether to use load or require during their test run. That would allow us to continue to use load in our own tests, but I'd have thought 80% of our regular users would be happy with require.

@emmakun
Copy link

emmakun commented Oct 31, 2016

Got it, and agree, I think it would be preferred 99% of time to use require instead of load, at least for the common user, so I will +1 the idea of a setting that defaults to require, but can be changed to use load instead if needed, just as @mattwynne suggested.

@djwgit
Copy link

djwgit commented Aug 18, 2017

run into the similar problem, one workaround is to put the base class file or included rb file in folders to have them in right order with folder name, say

  • _base folder has screen_base.rb
  • screen folder has screen1.rb, screen2.rb (all are sub class of screen_base)

this way, classes in _base folder are loaded first, then other folders.
it works, but... kind of...

@stale
Copy link

stale bot commented Nov 8, 2017

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

@stale stale bot added the ⌛ stale Will soon be closed by stalebot unless there is activity label Nov 8, 2017
@p0deje
Copy link

p0deje commented Nov 8, 2017

It's still pending and should not be closed.

@stale stale bot removed the ⌛ stale Will soon be closed by stalebot unless there is activity label Nov 8, 2017
@stale
Copy link

stale bot commented Jan 7, 2018

This issue has been automatically marked as stale because it has not had recent activity. It will be closed in a week if no further activity occurs.

@stale stale bot added the ⌛ stale Will soon be closed by stalebot unless there is activity label Jan 7, 2018
@p0deje
Copy link

p0deje commented Jan 8, 2018

Fun-fighting the bot.

@stale
Copy link

stale bot commented Jan 15, 2018

This issue has been automatically closed because of inactivity. You can support the Cucumber core team on opencollective.

@stale stale bot closed this as completed Jan 15, 2018
@p0deje
Copy link

p0deje commented Jan 15, 2018

Argh, the bot, why are you so ignorant?

@lock
Copy link

lock bot commented Jan 15, 2019

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 15, 2019
@mlvandijk mlvandijk added 🧷 pinned Tells Stalebot not to close this issue and removed ⌛ stale Will soon be closed by stalebot unless there is activity labels Jan 15, 2019
@mlvandijk
Copy link
Member

Added "Slow burner" label, to stop Stalebot from closing the issue.

@mlvandijk mlvandijk reopened this Jan 15, 2019
@cucumber cucumber unlocked this conversation Jan 15, 2019
@luke-hill
Copy link
Contributor

If @mattwynne or @roberts1000 could point me to some active links (Given the original links are 3 years old nearly), I'll have a little dive into this as most of the tidying up I did for rails is now in PR.

Not saying I'll get anywhere with this, but I'll take a stab

@roberts1000
Copy link
Author

Hi @luke-hill. I haven't followed the project in a while so I'm out the loop. I did a quick search for load (with a space at the end) and found this. It might get you started.

@luke-hill
Copy link
Contributor

@mattwynne Can I get you to look over a mini-spike I have for this, at some point in the next few days.

I've found it quite easy to switch these through configuration settings (I've got 2 different ways of doing this, so have pushed both into a branch), I'm just writing a couple of unit tests as an initial POC. I'll push something up to this repo tonight/tomorrow.

Obviously I have no idea if that's what you're happy with. I encountered the same issue as @roberts1000 - One unit test fails precisely because it prevents this functionality (Being able to only load once), and then a truckload of tests fail because aruba re-writes to the same temporary file names, which obviously won't be re-loaded in during the same ruby thread being used and as such about 50% of the features fail for undefined step

@luke-hill luke-hill self-assigned this May 8, 2019
@mattwynne
Copy link
Member

So I think the only thing I'd like to suggest here is that we make require the default, since this is going into a major release. It seems like load is more of a minority use-case, only really needed for situations where you're recycling one Cucumber process to run the same tests multiple times.

@mattwynne
Copy link
Member

I guess the main use-case here might be users of Zeus or Spring. I wonder if we can find any users of those tools who can give us their perspective?

@p0deje
Copy link

p0deje commented Sep 13, 2019

We currently monkey-patch Cucumber to avoid this issue:

module Monkey
  module Cucumber
    module Glue
      module RequireLoader
        def load_code_file(code_file)
          return unless File.extname(code_file) == '.rb'
          require File.expand_path(code_file)
        end
      end
    end
  end
end

Cucumber::Glue::RegistryAndMore.prepend(Monkey::Cucumber::Glue::RequireLoader)

The reason is that we use require_all gem to autoload all the support code and having Cucumber also load them ends up with constants being redfined.

@luke-hill
Copy link
Contributor

Ironically I write about this in the blog post coming out @p0deje, so I will be able to cover this use case.

I'll alter the usage accordingly @mattwynne in the PR and fix up the tests / merge with master

@luke-hill
Copy link
Contributor

This is now in master

@stripedpumpkin
Copy link

@luke-hill Do you know if this will fix things for cucumber-rails as well? Or is it necessary to do a similar change over there?

@luke-hill
Copy link
Contributor

As cucumber-rails uses cucumber then yes it "should" fix things.

Obviously with a variety of auto-loaders present things may get messy. Once we've released a cut of the gem which is compatible, we will update people accordingly.

@stripedpumpkin
Copy link

@luke-hill Looking forward to the release. Thanks for fixing this!

@luke-hill
Copy link
Contributor

@stripedpumpkin - I just realised that as we now have an rc cut of this gem, we could do a new version of cucumber-rails for this if you would like?

@stripedpumpkin
Copy link

@luke-hill That would be great. We could try it on a branch and see how that goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧷 pinned Tells Stalebot not to close this issue
Projects
None yet
Development

No branches or pull requests

9 participants