Improve execution time tracking for RSpec individual test examples. by ArturT · Pull Request #289 · KnapsackPro/knapsack_pro-ruby

ArturT · 2025-02-19T21:14:00Z

Story

Description

Related to the RSpec split by examples feature.

problem

Someone gives a custom set of paths to the Knapsack Pro using KNAPSACK_PRO_TEST_FILE_LIST_SOURCE_FILE.
There could be paths like a_spec.rb[1:1]. But the build_distributions#last returns tests which are not considered slow. a_spec.rb could be considered fast. We measure execution time for the full path a_spec.rb instead of a_spec.rb[1:1].

This can lead to misleading data because node 0 could measure a_spec.rb[1:1] as a_spec.rb taking 1 minute. But node 1 could measure a_spec.rb[1:2] as a_spec.rb taking 2 seconds.
Now we have two a_spec.rb paths with different timings. They are merged on the API side. We may end up with a build with a_spec.rb taking 2s while, in fact, it took much longer because it was split by examples between 2 nodes. We expect individual test examples to be measured.

solution

Individual test examples (a_spec.rb[1:1], a_spec.rb[1:2]) are measured from now on.

Checklist reminder

You added the changes to the UNRELEASED section of the CHANGELOG.md, including the needed bump (ie, patch, minor, major)
You follow the architecture outlined below for RSpec in Queue Mode, which is a work in progress (feel free to propose changes):
- 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 code and the extension to produce side effects, which are integration and e2e tested.

… the Queue API

… individual test examples

…s not blow up the code and fallback to []

…matter

…eue mode

https://app.circleci.com/pipelines/github/KnapsackPro/knapsack_pro-ruby/1647/workflows/328b5cdd-b957-4f09-bbcf-c4593ecde5e5/jobs/25408

… examples even though they are not considered as slow test files

…blow up the RSpec integration tests for regular mode instead of silently hide the error

3v0k4

I see that to keep things short you used "split by examples" or split_by_examples. I'd keep it consistent to not add another way of calling it, so: "split by test examples" or split_by_test_examples. If it's a throwaway variable inside a method I'm fine with a shortcut.

Example:

# ok (because if I grepped I would find the split line)

def do
  split = Env.split_by_test_examples_enabled?
  # ...
end

# not ok

@split
it "split by example"

Could you ping me after you apply the changes to take a second look please?

3v0k4 · 2025-02-20T09:52:03Z

+      def self.rspec_id_path?(path)
+        path_with_id_regex = /.+_spec\.rb\[.+\]$/
+
+        path&.match?(path_with_id_regex)
+      end


recommendation

Use the same naming:

Suggested change

def self.rspec_id_path?(path)

path_with_id_regex = /.+_spec\.rb\[.+\]$/

path&.match?(path_with_id_regex)

end

def self.rspec_id_path?(path)

id_path_regex = /.+_spec\.rb\[.+\]$/

path&.match?(id_path_regex)

end

I don't like that there are multiple callers and given the path&. it seems sometimes path is nil.

When does that happen?

suggestion

I'd go with re.match?(str) instead of the other way around. It reads better for me.

I don't like that there are multiple callers and given the path&. it seems sometimes path is nil.

When does that happen?

I don't know. I think it should not happen because a path should always be a string.

The rspec_id_path? method was moved in here from the lib/knapsack_pro/test_case_mergers/rspec_merger.rb, but originally it's definition comes from the API project:
https://github.com/KnapsackPro/knapsack-pro-api/commit/2ebfe5e133957df37be1d69c0ae999d8c7b0a7be/domain/project_domain/test_file/test_file_value.rb#diff-8

I applied changes: 868dc17

3v0k4 · 2025-02-20T09:56:31Z

+      end
+
+      def bind_regular_mode_time_tracker
+        return unless KnapsackPro::Config::Env.recording_enabled?


When is it true?

KnapsackPro::Config::Env.recording_enabled? is true when you use regular mode only.

I did a little refactoring because the following methods are a bit confusing:

KnapsackPro::Config::Env.recording_enabled? # is regular mode on KnapsackPro::Config::Env.queue_recording_enabled? # is queue mode on

I've renamed them to:

KnapsackPro::Config::Env.regular_mode? KnapsackPro::Config::Env.queue_mode?

3v0k4 · 2025-02-20T10:01:07Z

          @node_test_file_paths += test_file_paths
+
+          time_tracker = KnapsackPro::Formatters::TimeTrackerFetcher.call
+          time_tracker.scheduled_test_file_paths = @node_test_file_paths


Each time we set what was there before plus the new batch?

Yes.

Each time we override the time_tracker.scheduled_test_file_paths with @node_test_file_paths.

The @node_test_file_paths is updated on each batch pulled from the Queue API.

Co-authored-by: Riccardo <riccardo.odone@gmail.com>

Rules: file path (path without id) id path (path with id) path (either one)

…dividual_examples PR: KnapsackPro/knapsack_pro-ruby#289

3v0k4 · 2025-02-21T08:06:57Z

@ArturT I think the following would be clearer in the PR description:

Problem

Someone gives a set of paths to Knapsack Pro using KNAPSACK_PRO_TEST_FILE_LIST_SOURCE_FILE including a_spec.rb[1:1].

The build_distributions#last endpoint may return a_spec.rb (not a_spec.rb[1:1]), so the Knapsack Pro client measures the following execution times:

a_spec.rb takes as long as a_spec.rb[1:1]
a_spec.rb[1:1] takes 0 seconds (default time)

It may happen that:

node 0 measures a_spec.rb[1:1] as a_spec.rb taking 1 minute and a_spec.rb[1:1] taking 0 seconds
node 1 measures a_spec.rb[1:2] as a_spec.rb taking 2 seconds and a_spec.rb[1:2] taking 0 seconds

There are two a_spec.rb paths with different timings that are merged on the API side. We may end up with a build with a_spec.rb taking 2s while, in fact, it took much longer because it was split by examples between 2 nodes. We expect individual test examples to be measured.

Also, the dashboard shows:

a_spec.rb: 2 seconds
a_spec.rb[1:1]: 0 seconds
a_spec.rb[1:2]: 0 seconds

3v0k4

Fantastic work!

ArturT · 2025-02-21T09:14:04Z

@ArturT I think the following would be clearer in the PR description:

No. It's inaccurate.
I'll keep the original description.

Problem

Someone gives a set of paths to Knapsack Pro using KNAPSACK_PRO_TEST_FILE_LIST_SOURCE_FILE including a_spec.rb[1:1].

The build_distributions#last endpoint may return a_spec.rb (not a_spec.rb[1:1]), so the Knapsack Pro client measures the following execution times:

a_spec.rb takes as long as a_spec.rb[1:1]

a_spec.rb[1:1] takes 0 seconds (default time)

The build_distributions#last may return a_spec.rb and/or a_spec.rb[1:1]. It does not matter. The gem determines the slow test files based on whatever data it gets from the build_distributions#last API.

If the full test file path (a_spec.rb) is not determined as slow, but the Queue API returns a_spec.rb[1:1] then we measured execution time for the full path a_spec.rb. We expected a_spec.rb[1:1] to be measured.

Co-authored-by: Riccardo <riccardo.odone@gmail.com>

This reverts commit ed106ed.

3v0k4 · 2025-02-21T18:20:21Z

My point was that the description in this PR is confusing, that's why I tried to simplify it.

You may still want to update the description in case someone else gets here and tries to understand what's going on.

ArturT added 19 commits February 17, 2025 19:14

refactor: extract rspec_id_path? to RSpecAdapter

db69830

fix: measure test file path id execution times if get an id path from…

e668051

… the Queue API

require set to fix bug with missing Set.new in Ruby 3.1

b5791ee

require set in time tracker

8bbe358

fix(regular mode): scheduled id paths have execution time measured by…

5ecdc49

… individual test examples

if :@files_or_directories_to_run is no longer available in rspec let'…

8204d8a

…s not blow up the code and fallback to []

refactor: store @scheduled_test_file_paths inside of time tracker for…

66b4b3b

…matter

refactor: make measuring execution times work for both regular and qu…

56bec01

…eue mode

fix flay spec

bbd5f12

https://app.circleci.com/pipelines/github/KnapsackPro/knapsack_pro-ruby/1647/workflows/328b5cdd-b957-4f09-bbcf-c4593ecde5e5/jobs/25408

Update time_tracker_fetcher_spec.rb

a5a028b

add(spec): detects test execution times correctly for individual test…

57eeb57

… examples even though they are not considered as slow test files

if @files_or_directories_to_run is no longer set in RSpec then let's …

c75655b

…blow up the RSpec integration tests for regular mode instead of silently hide the error

no need for & in before(:suite)

f209a75

do not check if time_tracker exists in after(:suite)

8639cbf

add spec for scheduled_test_file_paths

cf4648a

Update time_tracker_specs.rb

d233881

show rspec version on CI for unit tests

f6b7129

Use the latest RSpec version to verify unit tests still work

1869a66

Update config.yml

8d6cea1

ArturT added the bug label Feb 19, 2025

ArturT added 3 commits February 19, 2025 22:16

Update CHANGELOG.md

452153e

Update time_tracker.rb

8eded91

Update rspec_runner_spec.rb

b3f23e4

ArturT commented Feb 19, 2025

View reviewed changes

Comment thread spec/knapsack_pro/config/env_spec.rb

3v0k4 reviewed Feb 20, 2025

View reviewed changes

ArturT and others added 5 commits February 20, 2025 14:13

Update .circleci/config.yml

e106cc8

Co-authored-by: Riccardo <riccardo.odone@gmail.com>

use regular_mode? method

aa71df4

refactor: keep regular_mode? and remove recording_enabled?

262dbcc

refactor: use KNAPSACK_PRO_REGULAR_MODE_ENABLED name

3b8ad7d

refactor: use KNAPSACK_PRO_QUEUE_MODE_ENABLED

b4a71db

ArturT and others added 11 commits February 20, 2025 15:44

use queue_mode? method name

3678449

make scheduled_test_file_paths private

cf73ee0

refactor: I'd inline the upsert. It's more clear to see it in one place.

9836704

Update spec/integration/runners/queue/rspec_runner_spec.rb

a3d9d27

Co-authored-by: Riccardo <riccardo.odone@gmail.com>

refactor: rspec_id_path?

868dc17

refactor: refactor test file paths naming

3eab4b0

Rules: file path (path without id) id path (path with id) path (either one)

Update time_tracker.rb

f38e82a

Update time_tracker.rb

8e44146

Update time_tracker.rb

ffe3659

keep full name: split_by_test_example

07727ae

use full name: RSpec split by test examples

7971efd

ArturT requested a review from 3v0k4 February 20, 2025 16:54

ArturT added a commit to KnapsackPro/rails-app-with-knapsack_pro that referenced this pull request Feb 20, 2025

Create bin/knapsack_pro_queue_rspec_split_by_test_examples_measure_in…

4b2abec

…dividual_examples PR: KnapsackPro/knapsack_pro-ruby#289

3v0k4 approved these changes Feb 21, 2025

View reviewed changes

Comment thread lib/knapsack_pro/adapters/rspec_adapter.rb Outdated

Comment thread lib/knapsack_pro/adapters/rspec_adapter.rb

Comment thread lib/knapsack_pro/formatters/time_tracker.rb Outdated

ArturT and others added 4 commits February 21, 2025 10:16

rename to id_path?

5ff9811

self.class

ed106ed

Update lib/knapsack_pro/formatters/time_tracker.rb

25e801c

Co-authored-by: Riccardo <riccardo.odone@gmail.com>

Revert "self.class"

41e7674

This reverts commit ed106ed.

ArturT merged commit 4e17620 into master Feb 21, 2025

ArturT deleted the improve-rspec-split-by-examples-execution-time-measures branch February 21, 2025 09:29

Conversation

ArturT commented Feb 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Story

Description

problem

solution

Checklist reminder

Uh oh!

Uh oh!

3v0k4 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

3v0k4 commented Feb 21, 2025

Problem

Uh oh!

3v0k4 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ArturT commented Feb 21, 2025

Problem

Uh oh!

3v0k4 commented Feb 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ArturT commented Feb 19, 2025 •

edited

Loading