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

Spike/altered loading strategy #1349

Merged
merged 11 commits into from
Sep 19, 2019
Merged

Conversation

luke-hill
Copy link
Contributor

Summary

Provide a way for cucumber to only load files once.

Details

Using a config switch, decide on whether cucumber should recurse through files potentially n times, or just once only.

Motivation and Context

This fixes an old problem here: #1043 - Where the user had procedural code which was being instantiated twice. A user should be able to specify the load order of a file, and "not" have the file loaded twice.

How Has This Been Tested?

Unit Tests have been added in the appropriate file.

Screenshots (if appropriate):

Types of changes

  • Refactor (code change that does not change external functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • I've added tests for my code
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

Outstanding questions FYI: @mattwynne / @roberts1000

  • Should we use the top level config or the config level which requires flags.
  • How should we advertise this?
  • Is there anything missing?

@luke-hill
Copy link
Contributor Author

I've coded this in two slightly different ways in different SHA's. Will remove the one / both we like/dislike

@mxygem
Copy link
Member

mxygem commented May 13, 2019

Nice work! WRT to your questions:

  • I think something like a --load-once flag could be suitable for invoking this.
  • If you're down for writing a blog post about the solution and how you did it, we could advertise it that way. :)
  • I saw that in Slack you said you were unsure if it was an issue for other languages: Is there any way you could find out? @mpkorstanje do you happen to know off-hand if this is something Java could benefit from as well?

@mpkorstanje
Copy link
Contributor

No. We have nothing comparable in Java.

@alexJunger
Copy link

Is there a reason for keeping load, given that this will make it to the new major (I assume)?
If configurable, I think the non-default compatibility option should be load, not require.

@luke-hill
Copy link
Contributor Author

I think that whilst this will be in a new major cut, there was no notice of deprecations beforehand, and this is quite a big change to the autoloader. I would rather keep the default as is and add a new config for v4 and then for v5 alter / remove / switch the default

@mattwynne
Copy link
Member

Looks good to me @luke-hill and I second @jaysonesmith's suggestion of a blog post when we do the release. Just make sure the changelog is up to date when you merge so you/we know what to build the blog post around.

@luke-hill luke-hill force-pushed the spike/altered_loading_strategy branch from fa5a1e8 to af0417b Compare June 5, 2019 07:02
@stale
Copy link

stale bot commented Aug 4, 2019

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 Aug 4, 2019
@luke-hill luke-hill added 🧷 pinned Tells Stalebot not to close this issue and removed ⌛ stale Will soon be closed by stalebot unless there is activity labels Aug 5, 2019
@luke-hill
Copy link
Contributor Author

As an update the following is required

A rebase (10min)
A review of my blog post (30min)
An update to the blog post, make it less like a German newspaper (1-2hours)
A post on slack/linkedin promoting the changes
A cut for a cucumber 4.0.0.rc2 - @mattwynne how would I go about this.
A changelog update (5mins)

@mattwynne
Copy link
Member

mattwynne commented Sep 12, 2019

@luke-hill how comes we're using the global Cucumber object for configuring this? If we tried to use an AfterConfiguration block, would that be too late?

@luke-hill
Copy link
Contributor Author

Couple of reasons

  • It felt right based on the original situation - It is something thats quite global to cucumber running (for ruby)
  • Using a Configuration object doesn't allow you set it in time, it kind of avoids it
  • I didn't try an AfterConfiguration block, but I have a feeling if it's similar to the Configuration objects it won't work (Guessing by the name)

Because all of the specs run inside a single ruby process/thread, the 'caching' state of load and require is done per test

In the previous iteration, we ran the caching test once(require) and the non-caching test twice(load)

Now we run the caching test twice, we need to ensure the names aren't identical to prevent test-leakage or pollution across tests
… which have been overwritten mid-thread

To fix this we need to ensure each file is uniquely named (Or keep this config setting on.
@luke-hill
Copy link
Contributor Author

Code wise this is all good now.

Issues outstanding

  • Tweak to blog from old keyword to new keyboard
  • Final review of blog
  • Altering the link in the changelog here

cc/ @mattwynne / @aslakhellesoy

@luke-hill
Copy link
Contributor Author

I'm going to merge this in now. And as/when we cut a version or release my blog post, we can quickly amend the changelog link

@luke-hill luke-hill merged commit 3e57420 into master Sep 19, 2019
@luke-hill luke-hill deleted the spike/altered_loading_strategy branch September 19, 2019 08:31
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

Successfully merging this pull request may close these issues.

5 participants