skip ScalarFunction test on Windows platform.#1139
Conversation
📝 WalkthroughWalkthroughUpdates test file to add Windows-specific skips for scalar function tests and includes RuboCop Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/duckdb_test/scalar_function_test.rb (1)
140-158: Consider extracting the scalar-function happy-path setup.Adding
Metrics/AbcSizehere is a sign that the register/query setup is getting too repetitive. A small helper forSET threads=1, parameter registration, and@con.register_scalar_functionwould shrink these tests instead of widening inline RuboCop suppressions.♻️ Possible helper shape
private def register_scalar_function(name:, return_type:, parameter_types: [], &block) `@con.execute`('SET threads=1') sf = DuckDB::ScalarFunction.new sf.name = name Array(parameter_types).each { |type| sf.add_parameter(type) } sf.return_type = return_type sf.set_function(&block) `@con.register_scalar_function`(sf) endThen this test becomes:
register_scalar_function( name: 'add_nums', return_type: DuckDB::LogicalType::INTEGER, parameter_types: [DuckDB::LogicalType::INTEGER, DuckDB::LogicalType::INTEGER] ) { |a, b| a + b }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/duckdb_test/scalar_function_test.rb` around lines 140 - 158, Extract the repeated scalar-function setup into a private helper (e.g., register_scalar_function) and use it from test_scalar_function_with_two_parameters: implement a helper that executes 'SET threads=1', constructs a DuckDB::ScalarFunction, sets sf.name, iterates Array(parameter_types) to call sf.add_parameter for each, sets sf.return_type, calls sf.set_function with the provided block, and calls `@con.register_scalar_function`(sf); then replace the inline setup in test_scalar_function_with_two_parameters with a call to register_scalar_function(name: 'add_nums', return_type: DuckDB::LogicalType::INTEGER, parameter_types: [DuckDB::LogicalType::INTEGER, DuckDB::LogicalType::INTEGER]) { |a, b| a + b } and remove the Metrics/AbcSize suppression.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/duckdb_test/scalar_function_test.rb`:
- Around line 57-59: The test method
test_register_scalar_function_raises_error_without_single_thread should be
skipped on Windows like test_register_scalar_function: add the same guard line
"skip 'Scalar functions with Ruby test' if Gem.win_platform?" at the start of
the test_register_scalar_function_raises_error_without_single_thread method so
the Ruby callback-registration error assertions (the calls to
`@con.register_scalar_function`(sf) and the assert_match checks for
/single-threaded execution/ and /SET threads=1/) are not run on Windows where
messages differ.
---
Nitpick comments:
In `@test/duckdb_test/scalar_function_test.rb`:
- Around line 140-158: Extract the repeated scalar-function setup into a private
helper (e.g., register_scalar_function) and use it from
test_scalar_function_with_two_parameters: implement a helper that executes 'SET
threads=1', constructs a DuckDB::ScalarFunction, sets sf.name, iterates
Array(parameter_types) to call sf.add_parameter for each, sets sf.return_type,
calls sf.set_function with the provided block, and calls
`@con.register_scalar_function`(sf); then replace the inline setup in
test_scalar_function_with_two_parameters with a call to
register_scalar_function(name: 'add_nums', return_type:
DuckDB::LogicalType::INTEGER, parameter_types: [DuckDB::LogicalType::INTEGER,
DuckDB::LogicalType::INTEGER]) { |a, b| a + b } and remove the Metrics/AbcSize
suppression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c95ed9c1-1ed2-4bd3-9d9f-909a59db97b3
📒 Files selected for processing (1)
test/duckdb_test/scalar_function_test.rb
| def test_register_scalar_function | ||
| skip 'Scalar functions with Ruby test' if Gem.win_platform? | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
from pathlib import Path
import re
text = Path("test/duckdb_test/scalar_function_test.rb").read_text()
pattern = re.compile(r'^\s*def\s+(test_[^\s(]+).*?(?=^\s*def\s+|\Z)', re.M | re.S)
for match in pattern.finditer(text):
name = match.group(1)
body = match.group(0)
if ("set_function" in body or "register_scalar_function" in body) and "Gem.win_platform?" not in body:
print(name)
PYRepository: suketa/ruby-duckdb
Length of output: 144
🏁 Script executed:
cd test/duckdb_test && sed -n '1,100p' scalar_function_test.rb | cat -nRepository: suketa/ruby-duckdb
Length of output: 3393
🏁 Script executed:
# Extract the two unguarded tests
cd test/duckdb_test && python3 - <<'PYEOF'
from pathlib import Path
import re
text = Path("scalar_function_test.rb").read_text()
lines = text.split('\n')
# Find line numbers for both tests
pattern = re.compile(r'^\s*def\s+(test_set_function|test_register_scalar_function_raises_error_without_single_thread)')
for i, line in enumerate(lines, 1):
if pattern.search(line):
print(f"Found '{pattern.search(line).group(1)}' at line {i}")
# Extract full test_set_function (should be around line 17)
print("\n=== test_set_function ===")
for i in range(16, 75):
print(f"{i+1:3d} | {lines[i]}")
# Extract full test_register_scalar_function_raises_error_without_single_thread (should be around line 75)
print("\n=== test_register_scalar_function_raises_error_without_single_thread ===")
for i in range(74, 95):
print(f"{i+1:3d} | {lines[i]}")
PYEOFRepository: suketa/ruby-duckdb
Length of output: 3039
🏁 Script executed:
# Get the full content with line numbers for context
cd test/duckdb_test && wc -l scalar_function_test.rbRepository: suketa/ruby-duckdb
Length of output: 89
🏁 Script executed:
# Let's look at lines around the two unguarded tests
cd test/duckdb_test && sed -n '17,100p' scalar_function_test.rb | cat -nRepository: suketa/ruby-duckdb
Length of output: 3022
Add Windows skip to test_register_scalar_function_raises_error_without_single_thread.
This test at lines 75-88 calls @con.register_scalar_function(sf) and asserts the error message matches /single-threaded execution/ and /SET threads=1/. If Ruby callback registration is not supported on Windows, the error will be raised but with a different message, causing the assert_match assertions to fail. Add skip 'Scalar functions with Ruby test' if Gem.win_platform? to the beginning of this test to match the guard pattern used in test_register_scalar_function.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/duckdb_test/scalar_function_test.rb` around lines 57 - 59, The test
method test_register_scalar_function_raises_error_without_single_thread should
be skipped on Windows like test_register_scalar_function: add the same guard
line "skip 'Scalar functions with Ruby test' if Gem.win_platform?" at the start
of the test_register_scalar_function_raises_error_without_single_thread method
so the Ruby callback-registration error assertions (the calls to
`@con.register_scalar_function`(sf) and the assert_match checks for
/single-threaded execution/ and /SET threads=1/) are not run on Windows where
messages differ.
1 participant
Summary by CodeRabbit