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

Stop calling RSpec::Core::Runner#run multiple times in Queue Mode #237

Merged
merged 405 commits into from
Feb 23, 2024

Conversation

ArturT
Copy link
Member

@ArturT ArturT commented Jan 17, 2024

story

https://trello.com/c/BcBhX2MW

based on PR

description

Refactor RSpec integration in Queue Mode to avoid calling the RSpec::Core::Runner#run multiple times.

This simplifies integration with RSpec and removes many edge cases. See the changelog with migration steps.

Other changes:

  • We added integration tests for RSpec to ensure all RSpec features work as expected. You get a quick feedback loop when running tests: spec/integration/runners/queue/rspec_runner_spec.rb.

  • You can uncomment the ENV['TEST__SHOW_DEBUG_LOG'] = 'true' line in the integration tests so that you see output from the RSpec. This is helpful when creating or editing integration tests.

  • We run integration RSpec tests on CI against different Ruby & RSpec core versions for backward compatibility. We want to support old RSpec versions even before 3.11.0. For example, RSpec 3.10.0 uses the rspec-core 3.10.2 versoin that does not have the rspec_is_quitting method. We want to ensure we respect that.

docs changes

change log preview

Preview changelog for this PR.

architecture

Architecture inspired by the talk: https://www.destroyallsoftware.com/talks/boundaries

  • Pure: lib/knapsack_pro/pure/queue/rspec_pure.rb contains pure functions that are unit tested.
  • Extension: lib/knapsack_pro/extensions/rspec_extension.rb encapsulates calls to RSpec internals and is integration and e2e tested.
  • Runner: lib/knapsack_pro/runners/queue/rspec_runner.rb invokes the pure and the extension to produce side effects, which are integration and e2e tested.

@ArturT ArturT added wip Work in progress RSpec labels Jan 17, 2024
def run(rspec_runner)
@rspec_runner = rspec_runner

rspec_runner.knapsack__setup
Copy link
Member Author

Choose a reason for hiding this comment

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

The rspec_runner is an instance of ::RSpec::Core::Runner. It has methods like knapsack__setup added by KnapsackPro::Extensions::RSpecExtension.setup!.

I'd like to go in the following direction based on https://www.youtube.com/watch?v=yTkzNHF6rMs

I can inject the rspec_runner value with mocked knapsack__* methods in the test environment to test different logic paths for core functionalities of the knapsack_pro.


The knapsack__* methods purpose is to use the RSpec internal methods that should not be used as public methods. RSpec has many public methods but they are marked as internal API with comments. We hide all the complexity of using the RSpec internal "private" methods inside of the knapsack__* methods.

I have isolation from RSpec internals and core knapsack logic. The core knapsack logic paths can be tested with unit tests and mocks.

The actual RSpec behavior can be tested when we run the knapsack_pro command on CI. For example, if something changes in the RSpec internals, then tests executed by the knapsack_pro command should fail on CI.

@3v0k4 Please let me know if you see something that is wrong and could be done better. Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ArturT

I believe the spirit of that talk is different. You may want to watch the same talk in a different version; in particular from minute 29:00: Gary does not test the shell and pretty much does not use any test doubles because of that.

The functional core contains only pure functions and all the conditionals, so it's a breeze to test. The imperative shell contains only dependencies, and can be tested with "one" integration test because there's pretty much one path through it, and if it works once, it works all the time.

Now, in this case, the situation is a bit more messy, so I wouldn't try to be that perfect. But I'd see what happens with:

  • Isolating pure functions and pushing all the conditionals inside them (or as many as possible), and unit testing them hard
  • Keeping RSpec code in the extension in a shape that closely matches the original RSpec code, so that we don't lose the mapping
  • Trying to have no conditionals outside of the shell, and test it at the integration level

The difficult part is finding the boundaries (and base the communication and values).

I hope this is helpful. Also, I'd be happy to pair program ro discuss some ideas as I don't have precise answers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you.

Copy link
Member Author

Choose a reason for hiding this comment

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

README.md Outdated Show resolved Hide resolved
ArturT and others added 23 commits February 22, 2024 19:44
Co-authored-by: Riccardo <riccardo.odone@gmail.com>
…knapsack_pro-ruby into refactor-queue-rspec-runner
Co-authored-by: Riccardo <riccardo.odone@gmail.com>
Co-authored-by: Riccardo <riccardo.odone@gmail.com>
Co-authored-by: Riccardo <riccardo.odone@gmail.com>
Co-authored-by: Riccardo <riccardo.odone@gmail.com>
Co-authored-by: Riccardo <riccardo.odone@gmail.com>
Co-authored-by: Riccardo <riccardo.odone@gmail.com>
Co-authored-by: Riccardo <riccardo.odone@gmail.com>
…knapsack_pro-ruby into refactor-queue-rspec-runner
…when the time tracker is not found and we should return an empty list of unexecuted test files
@ArturT ArturT requested a review from 3v0k4 February 22, 2024 23:03
Co-authored-by: Riccardo <riccardo.odone@gmail.com>
@ArturT ArturT merged commit d3568c5 into master Feb 23, 2024
45 checks passed
@ArturT ArturT deleted the refactor-queue-rspec-runner branch February 23, 2024 11:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants