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

Performance improvement: don't run rake knapsack_pro:rspec_test_example_detector when no slow test files are detected for RSpec #225

Merged
merged 11 commits into from
Sep 20, 2023

Conversation

ArturT
Copy link
Member

@ArturT ArturT commented Sep 19, 2023

Performance improvement: don't run rake knapsack_pro:rspec_test_example_detector when no slow test files are detected for RSpec.

Thanks to that, we improve performance and save a few or a dozen seconds (depending on the project's complexity).

Related issues

development testing

This PR change was tested with a bin script.

@ArturT ArturT changed the title Don't run rake knapsack_pro:rspec_test_example_detector when no slow test files detected for RSpec Performance improvement: don't run rake knapsack_pro:rspec_test_example_detector when no slow test files are detected for RSpec Sep 19, 2023
@ArturT ArturT requested a review from 3v0k4 September 19, 2023 17:44
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered moving the logic specific to an adapter inside the adapter_class?

We could prolly replace most of fast_and_slow_test_files_to_run and its if adapter_class == KnapsackPro::Adapters::RSpecAdapter && KnapsackPro::Config::Env.rspec_split_by_test_examples? with something like:

adapter_class.new.some_name_that_makes_sense(args)

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea. I extracted everything related to the RSpec to the adapter class. Other generic things stay in base allocator builder so that we have option to extend other adapters to also support test cases splitting in the future.

Comment on lines 105 to 107
# read the JSON report
detector = KnapsackPro::TestCaseDetectors::RSpecTestExampleDetector.new
test_file_example_paths = detector.test_file_example_paths
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# read the JSON report
detector = KnapsackPro::TestCaseDetectors::RSpecTestExampleDetector.new
test_file_example_paths = detector.test_file_example_paths
KnapsackPro::TestCaseDetectors::RSpecTestExampleDetector
.new
.test_file_example_paths

Copy link
Member Author

Choose a reason for hiding this comment

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

done

@@ -154,7 +154,7 @@
end

context 'when rake task to detect RSpec test examples works' do
let(:slow_test_files) { double(size: 5) }
let(:slow_test_files) { double(size: 5, empty?: false) }
Copy link
Contributor

Choose a reason for hiding this comment

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

Is slow_test_files an array? If so, I'd pass a real thing rather than a stub.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok. I use a real array now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Are we covering the empty?: true case?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes. It's covered now.

@@ -101,5 +87,24 @@ def get_slow_test_files
KnapsackPro.logger.debug("Detected #{slow_test_files.size} slow test files: #{slow_test_files.inspect}")
slow_test_files
end

def rspec_test_file_example_paths_for_slow_test_files(slow_test_files)
Copy link
Contributor

Choose a reason for hiding this comment

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

If you move this into the adapter, I guess you could do:

Suggested change
def rspec_test_file_example_paths_for_slow_test_files(slow_test_files)
def example_paths_for(test_files)

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@ArturT ArturT merged commit 325d316 into master Sep 20, 2023
@ArturT ArturT deleted the rspec-split-by-examples branch September 20, 2023 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants