Skip to content

Conversation

@3v0k4
Copy link
Contributor

@3v0k4 3v0k4 commented Jan 21, 2026

Description

See https://github.com/KnapsackPro/knapsack-pro-api/pull/1433

Checks

  • I added the changes to the UNRELEASED section of the CHANGELOG.md, including the needed bump (i.e., patch, minor, major)
  • I followed the architecture outlined below for RSpec in Queue Mode:
    • 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.

Copy link
Member

@ArturT ArturT left a comment

Choose a reason for hiding this comment

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

Great work!

end

if test_files.empty?
KnapsackPro.logger.warn("No test files were executed on this CI node.")
Copy link
Member

Choose a reason for hiding this comment

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

question

No message at all? I recall people saying that some nodes are not running tests. They assumed this was a bug. This message is intended to clarify that this is expected behaviour.

KNAPSACK_PRO_PROJECT_DIR=. \
KNAPSACK_PRO_CI_NODE_TOTAL=${2:-2} \
KNAPSACK_PRO_CI_NODE_INDEX=${1:-0} \
KNAPSACK_PRO_TEST_QUEUE_ID=${3:-$CI_BUILD_ID} \
Copy link
Member

Choose a reason for hiding this comment

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

question

Is this needed in the regular mode? I think it's not. You could remove it then.


it 'logs a warning' do
expect(described_class).to receive(:warn).with(
'You have set the environment variable KNAPSACK_PRO_TEST_QUEUE_ID to env:456 which could be automatically determined from the CI environment as abc:123.'
Copy link
Member

Choose a reason for hiding this comment

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

suggestion

This message could be more actionable. Tell the user to remove KNAPSACK_PRO_TEST_QUEUE_ID.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants