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

Knapsack pro 7.1.0 #12447

Merged
merged 3 commits into from
May 9, 2024
Merged

Conversation

mkllnk
Copy link
Member

@mkllnk mkllnk commented May 8, 2024

What? Why?

Knapsack Pro 7 changed the timing of calling RSpec's before-suite calls. Since they are, correctly, only called once before the suite, our before-suite call to compile assets when needed failed. In queue mode, there are no examples loaded yet when the before-suite hook is called and therefore we don't know yet if we need to compile assets or not.

Now we load the RSpec hooks early for Knapsack Pro to recognise them and we check for asset compilation before each batch.

What should we test?

  • Specs only.

Release notes

Changelog Category (reviewers may add a label for the release notes):

  • User facing changes
  • API changes (V0, V1, DFC or Webhook)
  • Technical changes only
  • Feature toggled

The title of the pull request will be included in the release notes.

Dependencies

Documentation updates

dependabot bot and others added 3 commits May 8, 2024 10:38
Bumps [knapsack_pro](https://github.com/KnapsackPro/knapsack_pro-ruby) from 6.0.4 to 7.1.0.
- [Changelog](https://github.com/KnapsackPro/knapsack_pro-ruby/blob/master/CHANGELOG.md)
- [Commits](KnapsackPro/knapsack_pro-ruby@v6.0.4...v7.1.0)

---
updated-dependencies:
- dependency-name: knapsack_pro
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>
Knapsack is replacing some of the RSpec logic, like running before-suite
and before-all hooks. Loading the spec helper early means that Knapsack
knows about the asset compilation hook when it loads the next batch of
specs.
The KnapsackPro queue mode can't predict which specs it will run. So we
need to check on each file (top-level describe block) which type of spec
it is and if we need to compile assets for it.

Old versions of KnapsackPro would execute the `before(:suite)` hooks on
every batch, but now it's only run once. With this change, we do the
same as before.
@mkllnk mkllnk self-assigned this May 8, 2024
@mkllnk mkllnk added the technical changes only These pull requests do not contain user facing changes and are grouped in release notes label May 8, 2024
@mkllnk mkllnk marked this pull request as ready for review May 8, 2024 04:11
Copy link
Collaborator

@rioug rioug left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good one, it's a surprisingly small change in the end !

@filipefurtad0 filipefurtad0 mentioned this pull request May 8, 2024
4 tasks
@mkllnk
Copy link
Member Author

mkllnk commented May 8, 2024

Hi @ArturT, nice to see you here. There may be an issue with the execution of before(:all) callbacks which forces us to load such callbacks before Knapsack. My theory of the problem:

  • Knapsack loads and runs before(:suite) once.
  • It fetches the next spec file from the queue.
  • Knapsack then runs before(:all) and requires the spec file. <-- I don't know this but it would explain why our hook is not executed unless we load it in the .rspec file.
  • The specs are executed.

Is the above theory right? Could Knapsack first load the spec file and then run the hooks including one that were just added by loading a spec helper? It's quite common to load additional spec helper files, possibly with new RSpec hooks, within certain spec files.

@dacook
Copy link
Member

dacook commented May 9, 2024

In queue mode, there are no examples loaded yet when the before-suite hook is called

Just a thought: I wonder if Knapsack could override the rspec examples method, and raise an exception explaining that you can't use it there.

@dacook
Copy link
Member

dacook commented May 9, 2024

Compared with another recent build on master, the overall runtime was reduced by 5min.
However these runtimes can vary a lot so that's hardly conclusive.

@dacook dacook merged commit cec1793 into openfoodfoundation:master May 9, 2024
52 checks passed
@ArturT
Copy link

ArturT commented May 9, 2024

Hi @mkllnk

Hi @ArturT, nice to see you here. There may be an issue with the execution of before(:all) callbacks which forces us to load such callbacks before Knapsack. My theory of the problem:

  • Knapsack loads and runs before(:suite) once.

Yes.

  • It fetches the next spec file from the queue.

Yes. The test files are loaded after before(:suite) has been executed.

  • Knapsack then runs before(:all) and requires the spec file. <-- I don't know this but it would explain why our hook is not executed unless we load it in the .rspec file.
  • The specs are executed.

Is the above theory right? Could Knapsack first load the spec file and then run the hooks including one that were just added by loading a spec helper? It's quite common to load additional spec helper files, possibly with new RSpec hooks, within certain spec files.

The rails_helper.rb and spec_helper.rb should be automatically loaded before any test is loaded by the knapsack_pro gem.

The change introduced in 7.0.1
https://github.com/KnapsackPro/knapsack_pro-ruby/blob/master/CHANGELOG.md#701

The before(:suite) hook should be registered and known to RSpec before any test file is loaded. You can define before(:suite) in spec_helper.rb or rails_helper.rb.

Tests are dynamically fetched in batches from Knapsack Pro Queue API and loaded after before(:suite) hook has already been executed. Your before(:suite) hook defined in a test file won't run because you loaded its definition too late (due to tests being dynamically loaded in multiple batches).

If you need to load a specific hook only once for a certain type of tests, you can look at this:
https://docs.knapsackpro.com/ruby/rspec/#load-code-only-once-for-a-specific-type-of-specs
It allows you to simulate before(:suite) behaviour that could be lazy load.

@ArturT
Copy link

ArturT commented May 9, 2024

In queue mode, there are no examples loaded yet when the before-suite hook is called

Just a thought: I wonder if Knapsack could override the rspec examples method, and raise an exception explaining that you can't use it there.

@dacook Perhaps we could detect that you defined before(:suite) in a test file that is loaded after the before(:suite) hook has already been executed. We could then warn the user that it's too late to define the hook.

@mkllnk mkllnk deleted the knapsack_pro-7.1.0 branch May 9, 2024 23:49
@mkllnk
Copy link
Member Author

mkllnk commented May 9, 2024

I totally understand the before(:suite) logic but I was wondering why the before(:all) hook was also called before the spec file is loaded.

Anyway, thank you for all the documentation. I didn't know about when_first_matching_example_defined which is a much better solution for our problem.

@ArturT
Copy link

ArturT commented May 10, 2024

I totally understand the before(:suite) logic but I was wondering why the before(:all) hook was also called before the spec file is loaded.

@mkllnk I think the reason is that your spec_helper.rb loads spec/base_spec_helper.rb which loads:

Dir[Rails.root.join("spec/support/**/*.rb")].sort.each { |f| require f }

It means spec/support/precompile_assets.rb (that has defined before(:all)) is loaded before any test file is fetched from the Queue API. As a result, the before(:all) hook is executed before test files.

The knapsack_pro gem loads spec_helper.rb and rails_helper.rb before loading tests.

@mkllnk
Copy link
Member Author

mkllnk commented May 14, 2024

I'm not sure if it's worth investigating further, but just for clarification: The problem was solved by loading the base_spec_helper.rb in the .rspec file. So the file was loaded even earlier. It's just registering the hook. From that I concluded that Knapsack ran those before-all hooks before the first queue file was loaded. Otherwise, it would have registered the hook when loading the spec file and then executed it before running it. But that didn't happen. As a test, I took all conditions out of that hook and it did not run when loaded from the spec file. It only ran when loaded in the .rspec file. That made me think that there's an old copy of the before-all hooks list called and not updated when new files are loaded. Anyway, we found a way that works for us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
technical changes only These pull requests do not contain user facing changes and are grouped in release notes
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants