diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e39f978..f9d5acf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -11,13 +11,14 @@ permissions: env: CARGO_TERM_COLOR: always + FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true jobs: check: name: Check runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 - run: cargo check @@ -26,7 +27,7 @@ jobs: name: Format runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - uses: dtolnay/rust-toolchain@stable with: components: rustfmt @@ -36,7 +37,7 @@ jobs: name: Clippy runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - uses: dtolnay/rust-toolchain@stable with: components: clippy @@ -50,7 +51,7 @@ jobs: matrix: os: [ubuntu-latest, macos-latest, windows-latest] steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 - run: cargo test @@ -59,7 +60,7 @@ jobs: name: Coverage runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - uses: dtolnay/rust-toolchain@stable - uses: Swatinem/rust-cache@v2 - uses: taiki-e/install-action@cargo-llvm-cov diff --git a/.github/workflows/docker.yml b/.github/workflows/docker.yml index 203be96..fdf0473 100644 --- a/.github/workflows/docker.yml +++ b/.github/workflows/docker.yml @@ -12,22 +12,23 @@ permissions: env: REGISTRY: ghcr.io IMAGE_NAME: ${{ github.repository }} + FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true jobs: docker: name: Build and Push runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - name: Set up QEMU - uses: docker/setup-qemu-action@v3 + uses: docker/setup-qemu-action@v4 - name: Set up Docker Buildx - uses: docker/setup-buildx-action@v3 + uses: docker/setup-buildx-action@v4 - name: Log in to GHCR - uses: docker/login-action@v3 + uses: docker/login-action@v4 with: registry: ${{ env.REGISTRY }} username: ${{ github.actor }} @@ -35,7 +36,7 @@ jobs: - name: Extract metadata id: meta - uses: docker/metadata-action@v5 + uses: docker/metadata-action@v6 with: images: ${{ env.REGISTRY }}/${{ env.IMAGE_NAME }} tags: | @@ -45,7 +46,7 @@ jobs: type=raw,value=latest,enable=${{ !contains(github.ref, '-') }} - name: Build and push - uses: docker/build-push-action@v6 + uses: docker/build-push-action@v7 with: context: . push: true diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 2daf4a3..c672e52 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -10,6 +10,7 @@ permissions: env: CARGO_TERM_COLOR: always + FORCE_JAVASCRIPT_ACTIONS_TO_NODE24: true jobs: build: @@ -36,7 +37,7 @@ jobs: artifact: rubyfast.exe steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - uses: dtolnay/rust-toolchain@stable with: targets: ${{ matrix.target }} @@ -75,7 +76,7 @@ jobs: Compress-Archive -Path ${{ matrix.artifact }} -DestinationPath ../../../rubyfast-${{ matrix.target }}.zip - name: Upload artifact - uses: actions/upload-artifact@v4 + uses: actions/upload-artifact@v7 with: name: rubyfast-${{ matrix.target }} path: rubyfast-${{ matrix.target }}.* @@ -85,10 +86,10 @@ jobs: needs: build runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - name: Download artifacts - uses: actions/download-artifact@v4 + uses: actions/download-artifact@v8 with: path: artifacts merge-multiple: true @@ -109,7 +110,7 @@ jobs: needs: release runs-on: ubuntu-latest steps: - - uses: actions/checkout@v4 + - uses: actions/checkout@v6 - uses: dtolnay/rust-toolchain@stable - run: cargo publish env: diff --git a/Cargo.lock b/Cargo.lock index 8d4c8eb..ad7248b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -381,7 +381,7 @@ dependencies = [ [[package]] name = "rubyfast" -version = "1.1.0" +version = "1.1.1" dependencies = [ "anyhow", "clap", diff --git a/src/analyzer.rs b/src/analyzer.rs index 5b019a4..605a9f6 100644 --- a/src/analyzer.rs +++ b/src/analyzer.rs @@ -172,4 +172,97 @@ mod tests { assert_eq!(byte_offset_to_line(&positions, 6), 2); assert_eq!(byte_offset_to_line(&positions, 12), 3); } + + #[test] + fn analyze_nonexistent_file_returns_error() { + let config = crate::config::Config::default(); + let result = super::analyze_file(std::path::Path::new("/nonexistent.rb"), &config); + assert!(result.is_err()); + } + + #[test] + fn analyze_file_with_parse_errors_no_ast_returns_error() { + let dir = tempfile::TempDir::new().unwrap(); + // This produces a fatal parse error with no recoverable AST + let file = dir.path().join("fatal.rb"); + std::fs::write(&file, "\x00\x01\x02").unwrap(); + let config = crate::config::Config::default(); + let result = super::analyze_file(&file, &config); + // May be Ok with empty offenses or Err depending on parser behavior + // Either way it should not panic + let _ = result; + } + + #[test] + fn analyze_file_with_recovered_ast_returns_empty() { + let dir = tempfile::TempDir::new().unwrap(); + let file = dir.path().join("recovered.rb"); + std::fs::write(&file, "def foo; end; def def; end").unwrap(); + let config = crate::config::Config::default(); + let result = super::analyze_file(&file, &config); + match result { + Ok(analysis) => assert!(analysis.offenses.is_empty()), + Err(_) => {} // Also acceptable — fatal parse error + } + } + + #[test] + fn analyze_empty_file_returns_empty() { + let dir = tempfile::TempDir::new().unwrap(); + let file = dir.path().join("empty.rb"); + std::fs::write(&file, "").unwrap(); + let config = crate::config::Config::default(); + let result = super::analyze_file(&file, &config).unwrap(); + assert!(result.offenses.is_empty()); + } + + #[test] + fn analyze_file_with_config_disabling_rule() { + let dir = tempfile::TempDir::new().unwrap(); + let file = dir.path().join("test.rb"); + std::fs::write(&file, "for x in [1]; end").unwrap(); + let config = + crate::config::Config::parse_yaml("speedups:\n for_loop_vs_each: false\n").unwrap(); + let result = super::analyze_file(&file, &config).unwrap(); + assert!(result.offenses.is_empty()); + } + + #[test] + fn analyze_file_with_inline_disable() { + let dir = tempfile::TempDir::new().unwrap(); + let file = dir.path().join("test.rb"); + std::fs::write( + &file, + "for x in [1]; end # rubyfast:disable for_loop_vs_each\n", + ) + .unwrap(); + let config = crate::config::Config::default(); + let result = super::analyze_file(&file, &config).unwrap(); + assert!(result.offenses.is_empty()); + } + + #[test] + fn walk_node_block_with_non_send_call() { + // A numblock (numbered params) has a different call structure + let dir = tempfile::TempDir::new().unwrap(); + let file = dir.path().join("test.rb"); + std::fs::write(&file, "arr.map { |x| x.to_s }").unwrap(); + let config = crate::config::Config::default(); + let result = super::analyze_file(&file, &config).unwrap(); + // Should find block_vs_symbol_to_proc + assert!(!result.offenses.is_empty()); + } + + #[test] + fn walk_node_nested_for_inside_method() { + let dir = tempfile::TempDir::new().unwrap(); + let file = dir.path().join("test.rb"); + std::fs::write(&file, "def foo\n for x in [1,2]; puts x; end\nend\n").unwrap(); + let config = crate::config::Config::default(); + let result = super::analyze_file(&file, &config).unwrap(); + assert!(result + .offenses + .iter() + .any(|o| o.kind == crate::offense::OffenseKind::ForLoopVsEach)); + } } diff --git a/src/ast_helpers.rs b/src/ast_helpers.rs index 305edcd..70f15be 100644 --- a/src/ast_helpers.rs +++ b/src/ast_helpers.rs @@ -194,6 +194,566 @@ pub fn for_each_child<'a>(node: &'a Node, mut f: impl FnMut(&'a Node)) { visit_children(node, &mut f); } +#[cfg(test)] +mod tests { + use super::*; + use lib_ruby_parser::Parser; + + fn parse(source: &[u8]) -> Option> { + Parser::new(source.to_vec(), Default::default()) + .do_parse() + .ast + } + + fn count_children(node: &Node) -> usize { + let mut count = 0; + for_each_child(node, |_| count += 1); + count + } + + /// Recursively count all nodes in the AST. + fn count_all_nodes(node: &Node) -> usize { + let mut count = 1; + for_each_child(node, |child| count += count_all_nodes(child)); + count + } + + #[test] + fn byte_offset_to_line_basic() { + let positions = vec![5, 11]; + assert_eq!(byte_offset_to_line(&positions, 0), 1); + assert_eq!(byte_offset_to_line(&positions, 5), 1); // exact match + assert_eq!(byte_offset_to_line(&positions, 6), 2); + assert_eq!(byte_offset_to_line(&positions, 12), 3); + } + + #[test] + fn byte_offset_to_line_empty() { + assert_eq!(byte_offset_to_line(&[], 0), 1); + assert_eq!(byte_offset_to_line(&[], 100), 1); + } + + #[test] + fn receiver_is_send_with_name_works() { + let ast = parse(b"a.foo.bar").unwrap(); + if let Node::Send(s) = ast.as_ref() { + assert!(receiver_is_send_with_name(&s.recv, "foo")); + assert!(!receiver_is_send_with_name(&s.recv, "baz")); + } + } + + #[test] + fn receiver_is_send_with_name_none() { + assert!(!receiver_is_send_with_name(&None, "foo")); + } + + #[test] + fn receiver_as_send_works() { + let ast = parse(b"a.foo.bar").unwrap(); + if let Node::Send(s) = ast.as_ref() { + let inner = receiver_as_send(&s.recv).unwrap(); + assert_eq!(inner.method_name, "foo"); + } + } + + #[test] + fn receiver_as_send_not_send() { + assert!(receiver_as_send(&None).is_none()); + } + + #[test] + fn block_call_as_send_works() { + let ast = parse(b"arr.map { |x| x }").unwrap(); + if let Node::Block(b) = ast.as_ref() { + let send = block_call_as_send(b).unwrap(); + assert_eq!(send.method_name, "map"); + } + } + + #[test] + fn has_block_pass_works() { + let ast = parse(b"arr.map(&:to_s)").unwrap(); + if let Node::Send(s) = ast.as_ref() { + assert!(has_block_pass(&s.args)); + } + } + + #[test] + fn has_block_pass_without() { + let ast = parse(b"arr.map(1)").unwrap(); + if let Node::Send(s) = ast.as_ref() { + assert!(!has_block_pass(&s.args)); + } + } + + #[test] + fn arg_count_without_block_pass_works() { + let ast = parse(b"arr.select(&:odd?).first").unwrap(); + if let Node::Send(s) = ast.as_ref() { + assert_eq!(arg_count_without_block_pass(&s.args), 0); + } + } + + #[test] + fn is_single_char_string_works() { + let ast = parse(b"'x'").unwrap(); + assert!(is_single_char_string(&ast)); + let ast2 = parse(b"'xy'").unwrap(); + assert!(!is_single_char_string(&ast2)); + } + + #[test] + fn is_single_char_string_not_string() { + let ast = parse(b"42").unwrap(); + assert!(!is_single_char_string(&ast)); + } + + #[test] + fn receiver_is_range_irange() { + let ast = parse(b"(1..10).include?(5)").unwrap(); + if let Node::Send(s) = ast.as_ref() { + assert!(receiver_is_range(&s.recv)); + } + } + + #[test] + fn receiver_is_range_erange() { + let ast = parse(b"(1...10).include?(5)").unwrap(); + if let Node::Send(s) = ast.as_ref() { + assert!(receiver_is_range(&s.recv)); + } + } + + #[test] + fn receiver_is_range_not_range() { + let ast = parse(b"[1].include?(5)").unwrap(); + if let Node::Send(s) = ast.as_ref() { + assert!(!receiver_is_range(&s.recv)); + } + } + + #[test] + fn is_primitive_covers_types() { + assert!(is_primitive(&parse(b"42").unwrap())); + assert!(is_primitive(&parse(b"3.14").unwrap())); + assert!(is_primitive(&parse(b"'s'").unwrap())); + assert!(is_primitive(&parse(b":sym").unwrap())); + assert!(is_primitive(&parse(b"true").unwrap())); + assert!(is_primitive(&parse(b"false").unwrap())); + assert!(is_primitive(&parse(b"nil").unwrap())); + assert!(is_primitive(&parse(b"[]").unwrap())); + assert!(is_primitive(&parse(b"{}").unwrap())); + assert!(is_primitive(&parse(b"1..5").unwrap())); + assert!(is_primitive(&parse(b"1...5").unwrap())); + assert!(!is_primitive(&parse(b"x").unwrap())); + } + + #[test] + fn first_arg_is_single_pair_hash_kwargs() { + let ast = parse(b"h.merge!(a: 1)").unwrap(); + if let Node::Send(s) = ast.as_ref() { + assert!(first_arg_is_single_pair_hash(&s.args)); + } + } + + #[test] + fn first_arg_is_single_pair_hash_explicit() { + let ast = parse(b"h.merge!({a: 1})").unwrap(); + if let Node::Send(s) = ast.as_ref() { + assert!(first_arg_is_single_pair_hash(&s.args)); + } + } + + #[test] + fn first_arg_is_single_pair_hash_multi() { + let ast = parse(b"h.merge!(a: 1, b: 2)").unwrap(); + if let Node::Send(s) = ast.as_ref() { + assert!(!first_arg_is_single_pair_hash(&s.args)); + } + } + + #[test] + fn first_arg_is_single_pair_hash_not_hash() { + let ast = parse(b"h.merge!(x)").unwrap(); + if let Node::Send(s) = ast.as_ref() { + assert!(!first_arg_is_single_pair_hash(&s.args)); + } + } + + #[test] + fn is_int_one_works() { + assert!(is_int_one(&parse(b"1").unwrap())); + assert!(!is_int_one(&parse(b"2").unwrap())); + assert!(!is_int_one(&parse(b"'1'").unwrap())); + } + + #[test] + fn block_arg_names_single() { + let ast = parse(b"arr.map { |x| x }").unwrap(); + if let Node::Block(b) = ast.as_ref() { + let names = block_arg_names(&b.args); + assert_eq!(names, vec!["x".to_string()]); + } + } + + #[test] + fn block_arg_names_none() { + let names = block_arg_names(&None); + assert!(names.is_empty()); + } + + #[test] + fn def_block_arg_name_present() { + let ast = parse(b"def foo(&block); end").unwrap(); + if let Node::Def(d) = ast.as_ref() { + assert_eq!(def_block_arg_name(d), Some("block".to_string())); + } + } + + #[test] + fn def_block_arg_name_absent() { + let ast = parse(b"def foo(x); end").unwrap(); + if let Node::Def(d) = ast.as_ref() { + assert_eq!(def_block_arg_name(d), None); + } + } + + #[test] + fn def_regular_arg_count_works() { + let ast = parse(b"def foo(a, b); end").unwrap(); + if let Node::Def(d) = ast.as_ref() { + assert_eq!(def_regular_arg_count(d), 2); + } + } + + #[test] + fn def_regular_arg_count_no_args() { + let ast = parse(b"def foo; end").unwrap(); + if let Node::Def(d) = ast.as_ref() { + assert_eq!(def_regular_arg_count(d), 0); + } + } + + #[test] + fn def_first_arg_name_works() { + let ast = parse(b"def foo(bar); end").unwrap(); + if let Node::Def(d) = ast.as_ref() { + assert_eq!(def_first_arg_name(d), Some("bar".to_string())); + } + } + + #[test] + fn def_first_arg_name_no_args() { + let ast = parse(b"def foo; end").unwrap(); + if let Node::Def(d) = ast.as_ref() { + assert_eq!(def_first_arg_name(d), None); + } + } + + #[test] + fn str_contains_def_in_string() { + let ast = parse(b"\"def foo\"").unwrap(); + assert!(str_contains_def(&ast)); + } + + #[test] + fn str_contains_def_no_def() { + let ast = parse(b"\"hello\"").unwrap(); + assert!(!str_contains_def(&ast)); + } + + #[test] + fn str_contains_def_not_string() { + let ast = parse(b"42").unwrap(); + assert!(!str_contains_def(&ast)); + } + + #[test] + fn str_contains_def_heredoc() { + let ast = parse(b"<<~RUBY\ndef foo\nRUBY\n").unwrap(); + assert!(str_contains_def(&ast)); + } + + #[test] + fn body_expressions_none() { + assert!(body_expressions(&None).is_empty()); + } + + #[test] + fn body_expressions_single() { + let ast = parse(b"def foo; 42; end").unwrap(); + if let Node::Def(d) = ast.as_ref() { + let exprs = body_expressions(&d.body); + assert_eq!(exprs.len(), 1); + } + } + + #[test] + fn body_expressions_begin() { + let ast = parse(b"def foo; 1; 2; 3; end").unwrap(); + if let Node::Def(d) = ast.as_ref() { + let exprs = body_expressions(&d.body); + assert_eq!(exprs.len(), 3); + } + } + + #[test] + fn node_children_matches_for_each_child() { + let ast = parse(b"a + b").unwrap(); + let children = node_children(&ast); + let mut count = 0; + for_each_child(&ast, |_| count += 1); + assert_eq!(children.len(), count); + } + + // Exercise many AST node types through visit_children for coverage + #[test] + fn visit_children_comprehensive() { + let sources: &[&[u8]] = &[ + // Alias + b"alias new_method old_method", + // And, Or + b"a && b || c", + // AndAsgn, OrAsgn + b"x &&= 1; y ||= 2", + // Array, ArrayPattern + b"[1, 2, 3]", + // Begin, Break, Next, Return + b"begin; break 1; end", + b"loop { next }", + // Case, When + b"case x; when 1; 'a'; when 2; 'b'; else 'c'; end", + // Casgn + b"FOO = 1", + // Class + b"class Foo < Bar; end", + // Const + b"Foo::Bar", + // CSend + b"x&.foo(1)", + // Cvasgn, Cvar + b"@@x = 1; @@x", + // Def, Defs + b"def foo(a); end", + b"def self.bar; end", + // Defined + b"defined?(x)", + // Dstr, Dsym + b"\"hello #{world}\"", + b":\"sym_#{x}\"", + // EFlipFlop, IFlipFlop + // Ensure + b"begin; 1; ensure; 2; end", + // Erange, Irange + b"1...10; 1..10", + // For + b"for x in [1]; end", + // Gvasgn, Gvar + b"$x = 1; $x", + // Hash, Pair + b"{a: 1, b: 2}", + // Heredoc + b"<<~HERE\nhello\nHERE\n", + // If, IfMod, IfTernary + b"if true; 1; else; 2; end", + b"x = 1 if true", + b"true ? 1 : 2", + // Index, IndexAsgn + b"a[0]; a[0] = 1", + // Ivasgn, Ivar + b"@x = 1; @x", + // Kwargs, Kwsplat + b"foo(a: 1, **opts)", + // KwBegin + b"begin; 1; rescue; 2; end", + // Kwoptarg, Kwrestarg + b"def foo(a: 1, **rest); end", + // Lvasgn + b"x = 42", + // Masgn, Mlhs + b"a, b = 1, 2", + // Module + b"module Foo; end", + // Next, Return + b"def foo; return 1; end", + // Numblock + b"arr.map { _1.to_s }", + // OpAsgn + b"x += 1", + // Optarg + b"def foo(a = 1); end", + // Pin (pattern matching) + b"case x; in ^y; end", + // Postexe, Preexe + b"END { 1 }", + b"BEGIN { 1 }", + // Procarg0 (block with single destructured arg) + b"arr.each { |(a)| a }", + // Regexp, RegOpt + b"/foo/i", + // Rescue, RescueBody + b"begin; rescue StandardError => e; end", + // SClass + b"class << self; end", + // Send + b"foo.bar(1, 2)", + // Splat + b"foo(*args)", + // Str + b"'hello'", + // Super + b"def foo; super(1); end", + // Undef + b"undef :foo", + // Until, While + b"until false; end", + b"while true; break; end", + // Yield + b"def foo; yield 1; end", + // Xstr, XHeredoc + b"`echo hi`", + // MatchCurrentLine + b"if /pattern/; end", + // Block, BlockPass + b"arr.select(&:odd?)", + // FindPattern, InPattern + b"case x; in [1, *rest, 2]; end", + // MatchAlt, MatchAs + b"case x; in 1 | 2 => y; end", + // MatchPattern, MatchPatternP + b"x in [1, 2]", + b"x in [1, 2] rescue false", + // MatchNilPattern, MatchVar + b"case x; in **nil; end", + b"case x; in {a:}; end", + // MatchWithLvasgn + b"/(?.)/ =~ str", + // HashPattern, ConstPattern + b"case x; in Foo[a:]; end", + // MatchRest + b"case x; in [*, 1]; end", + // WhilePost + b"begin; 1; end while true", + // UntilPost + b"begin; 1; end until true", + // UnlessGuard + b"case x; in 1 unless false; end", + // EFlipFlop (exclusive) + b"if (a == 1)...(b == 2); end", + // IFlipFlop (inclusive) + b"if (a == 1)..(b == 2); end", + // Rational, Complex + b"1r", + b"1i", + // BackRef, NthRef + b"$~ ; $1", + // Redo, Retry + b"begin; retry; rescue; end", + // Self + b"self", + // ZSuper + b"def foo; super; end", + // Lambda + b"-> { 1 }", + // Encoding, File, Line + b"__ENCODING__", + b"__FILE__", + b"__LINE__", + // XHeredoc + b"<<~`CMD`\necho hi\nCMD\n", + // ArrayPatternWithTail + b"case x; in [1, 2,]; end", + // ForwardArg, ForwardedArgs + b"def foo(...); bar(...); end", + // Kwarg + b"def foo(a:); end", + // Kwnilarg + b"def foo(**nil); end", + // Shadowarg + b"arr.each { |x; y| y }", + // Restarg + b"def foo(*args); end", + ]; + + for source in sources { + if let Some(ast) = parse(source) { + let total = count_all_nodes(&ast); + assert!( + total > 0, + "No nodes in AST for {:?}", + std::str::from_utf8(source) + ); + } + } + } + + #[test] + fn visit_children_pattern_matching() { + // These exercise pattern matching AST nodes specifically + let sources: &[&[u8]] = &[ + // MatchPattern (in operator) + b"1 in Integer", + // MatchPatternP (case/in with guard) + b"case 1; in Integer if true; end", + // IfGuard + b"case 1; in x if x > 0; end", + // UnlessGuard + b"case 1; in x unless x < 0; end", + // FindPattern + b"case [1,2,3]; in [*, 2, *]; end", + // HashPattern + b"case {a: 1}; in {a: Integer}; end", + // ConstPattern + b"case x; in Foo(1); end", + // MatchNilPattern + b"case {a: 1}; in **nil; end", + // MatchVar + b"case 1; in x; end", + // MatchRest + b"case [1,2]; in [Integer, *rest]; end", + // MatchAlt + b"case 1; in 1 | 2; end", + // MatchAs + b"case 1; in Integer => x; end", + // MatchWithLvasgn (regex named capture) + b"/(?.)/ =~ 'x'", + // Pin + b"x = 1; case 2; in ^x; end", + ]; + for source in sources { + if let Some(ast) = parse(source) { + let total = count_all_nodes(&ast); + assert!(total > 0, "No nodes for {:?}", std::str::from_utf8(source)); + } + } + } + + #[test] + fn visit_children_leaf_nodes() { + // Leaf nodes should have 0 children + let leaf_sources: &[&[u8]] = &[ + b"42", // Int + b"3.14", // Float + b"'s'", // Str + b":sym", // Sym + b"true", // True + b"false", // False + b"nil", // Nil + b"x", // Lvar + ]; + + for source in leaf_sources { + let ast = parse(source).unwrap(); + assert_eq!( + count_children(&ast), + 0, + "Expected 0 children for {:?}", + std::str::from_utf8(source) + ); + } + } +} + fn visit_opt<'a>(opt: &'a Option>, f: &mut impl FnMut(&'a Node)) { if let Some(n) = opt.as_deref() { f(n); diff --git a/src/comment_directives.rs b/src/comment_directives.rs index 7934fae..df9acc3 100644 --- a/src/comment_directives.rs +++ b/src/comment_directives.rs @@ -305,4 +305,74 @@ mod tests { let set = parse_and_build(source); assert!(!set.is_disabled(1, OffenseKind::ShuffleFirstVsSample)); } + + #[test] + fn disable_next_line_all() { + let source = "# rubyfast:disable-next-line all\nx = [].shuffle.first\ny = 1\n"; + let set = parse_and_build(source); + assert!(set.is_disabled(2, OffenseKind::ShuffleFirstVsSample)); + assert!(set.is_disabled(2, OffenseKind::ForLoopVsEach)); + assert!(!set.is_disabled(3, OffenseKind::ShuffleFirstVsSample)); + } + + #[test] + fn block_disable_all_and_enable_all() { + let source = "# rubyfast:disable all\nx = 1\ny = 2\n# rubyfast:enable all\nz = 3\n"; + let set = parse_and_build(source); + assert!(set.is_disabled(2, OffenseKind::ShuffleFirstVsSample)); + assert!(set.is_disabled(3, OffenseKind::ForLoopVsEach)); + assert!(!set.is_disabled(5, OffenseKind::ShuffleFirstVsSample)); + } + + #[test] + fn multiple_rules_in_block_disable() { + let source = "# rubyfast:disable shuffle_first_vs_sample, for_loop_vs_each\nx = 1\n# rubyfast:enable shuffle_first_vs_sample, for_loop_vs_each\ny = 2\n"; + let set = parse_and_build(source); + assert!(set.is_disabled(2, OffenseKind::ShuffleFirstVsSample)); + assert!(set.is_disabled(2, OffenseKind::ForLoopVsEach)); + assert!(!set.is_disabled(4, OffenseKind::ShuffleFirstVsSample)); + assert!(!set.is_disabled(4, OffenseKind::ForLoopVsEach)); + } + + #[test] + fn unclosed_block_disable_all_extends_to_eof() { + let source = "# rubyfast:disable all\nx = 1\ny = 2\n"; + let set = parse_and_build(source); + assert!(set.is_disabled(2, OffenseKind::ShuffleFirstVsSample)); + assert!(set.is_disabled(3, OffenseKind::GsubVsTr)); + } + + #[test] + fn empty_disable_directive_ignored() { + let source = "x = 1 # rubyfast:disable\n"; + let set = parse_and_build(source); + assert!(!set.is_disabled(1, OffenseKind::ShuffleFirstVsSample)); + } + + #[test] + fn empty_enable_directive_ignored() { + let source = "# rubyfast:enable\n"; + let set = parse_and_build(source); + assert!(!set.is_disabled(1, OffenseKind::ShuffleFirstVsSample)); + } + + #[test] + fn is_trailing_at_start_of_file() { + let source = b"# comment\nx = 1\n"; + assert!(!is_trailing_comment(source, 0)); + } + + #[test] + fn unrecognized_directive_action_ignored() { + let source = "x = 1 # rubyfast:freeze all\n"; + let set = parse_and_build(source); + assert!(!set.is_disabled(1, OffenseKind::ShuffleFirstVsSample)); + } + + #[test] + fn enable_without_matching_disable_is_noop() { + let source = "# rubyfast:enable for_loop_vs_each\nfor x in [1]; end\n"; + let set = parse_and_build(source); + assert!(!set.is_disabled(2, OffenseKind::ForLoopVsEach)); + } } diff --git a/src/config.rs b/src/config.rs index d8cdd4f..7cd1103 100644 --- a/src/config.rs +++ b/src/config.rs @@ -114,4 +114,80 @@ mod tests { assert_eq!(config.exclude_patterns.len(), 2); assert_eq!(config.exclude_patterns[0], "vendor/**/*.rb"); } + + #[test] + fn all_speedups_true_enables_all() { + let yaml = "speedups:\n for_loop_vs_each: true\n gsub_vs_tr: true\n"; + let config = Config::parse_yaml(yaml).unwrap(); + assert!(config.is_enabled(OffenseKind::ForLoopVsEach)); + assert!(config.is_enabled(OffenseKind::GsubVsTr)); + } + + #[test] + fn unknown_speedup_key_ignored() { + let yaml = "speedups:\n made_up_rule: false\n"; + let config = Config::parse_yaml(yaml).unwrap(); + for kind in OffenseKind::all() { + assert!(config.is_enabled(*kind)); + } + } + + #[test] + fn invalid_yaml_returns_error() { + let result = Config::parse_yaml("speedups: [invalid"); + assert!(result.is_err()); + } + + #[test] + fn load_rubyfast_yml() { + let dir = tempfile::TempDir::new().unwrap(); + std::fs::write( + dir.path().join(".rubyfast.yml"), + "speedups:\n gsub_vs_tr: false\n", + ) + .unwrap(); + let config = Config::load(dir.path()).unwrap(); + assert!(!config.is_enabled(OffenseKind::GsubVsTr)); + } + + #[test] + fn load_fasterer_yml_fallback() { + let dir = tempfile::TempDir::new().unwrap(); + std::fs::write( + dir.path().join(".fasterer.yml"), + "speedups:\n for_loop_vs_each: false\n", + ) + .unwrap(); + let config = Config::load(dir.path()).unwrap(); + assert!(!config.is_enabled(OffenseKind::ForLoopVsEach)); + } + + #[test] + fn load_no_config_returns_default() { + let dir = tempfile::TempDir::new().unwrap(); + let config = Config::load(dir.path()).unwrap(); + for kind in OffenseKind::all() { + assert!(config.is_enabled(*kind)); + } + } + + #[test] + fn from_file_nonexistent_returns_error() { + let result = Config::from_file(std::path::Path::new("/nonexistent/.rubyfast.yml")); + assert!(result.is_err()); + } + + #[test] + fn load_walks_up_parent_directories() { + let dir = tempfile::TempDir::new().unwrap(); + std::fs::write( + dir.path().join(".rubyfast.yml"), + "speedups:\n gsub_vs_tr: false\n", + ) + .unwrap(); + let sub = dir.path().join("nested").join("deep"); + std::fs::create_dir_all(&sub).unwrap(); + let config = Config::load(&sub).unwrap(); + assert!(!config.is_enabled(OffenseKind::GsubVsTr)); + } } diff --git a/src/file_traverser.rs b/src/file_traverser.rs index 873f145..7fdbc9e 100644 --- a/src/file_traverser.rs +++ b/src/file_traverser.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::path::{Path, PathBuf}; use rayon::prelude::*; @@ -79,7 +80,7 @@ fn collect_ruby_files(path: &Path) -> Vec { } /// Expand exclude patterns relative to a base path, pre-canonicalizing results. -fn collect_excluded_files(patterns: &[String], base: &Path) -> Vec { +fn collect_excluded_files(patterns: &[String], base: &Path) -> HashSet { patterns .iter() .flat_map(|pattern| { @@ -103,7 +104,176 @@ fn collect_excluded_files(patterns: &[String], base: &Path) -> Vec { } /// Check if a file should be excluded. -fn is_excluded(file: &Path, excluded: &[PathBuf]) -> bool { +fn is_excluded(file: &Path, excluded: &HashSet) -> bool { let file_canonical = file.canonicalize().unwrap_or_else(|_| file.to_path_buf()); excluded.contains(&file_canonical) } + +#[cfg(test)] +mod tests { + use super::*; + use std::fs; + use tempfile::TempDir; + + #[test] + fn traversal_result_total_offenses() { + let result = TraversalResult { + results: vec![ + crate::analyzer::AnalysisResult { + path: "a.rb".to_string(), + offenses: vec![ + crate::offense::Offense::new(crate::offense::OffenseKind::GsubVsTr, 1), + crate::offense::Offense::new(crate::offense::OffenseKind::GsubVsTr, 2), + ], + }, + crate::analyzer::AnalysisResult { + path: "b.rb".to_string(), + offenses: vec![crate::offense::Offense::new( + crate::offense::OffenseKind::GsubVsTr, + 1, + )], + }, + ], + parse_errors: vec![], + files_inspected: 2, + }; + assert_eq!(result.total_offenses(), 3); + assert!(result.has_offenses()); + } + + #[test] + fn traversal_result_no_offenses() { + let result = TraversalResult { + results: vec![], + parse_errors: vec![], + files_inspected: 0, + }; + assert_eq!(result.total_offenses(), 0); + assert!(!result.has_offenses()); + } + + #[test] + fn collect_ruby_files_single_file() { + let dir = TempDir::new().unwrap(); + let file = dir.path().join("test.rb"); + fs::write(&file, "x = 1").unwrap(); + let files = collect_ruby_files(&file); + assert_eq!(files.len(), 1); + } + + #[test] + fn collect_ruby_files_directory() { + let dir = TempDir::new().unwrap(); + fs::write(dir.path().join("a.rb"), "x = 1").unwrap(); + fs::write(dir.path().join("b.rb"), "y = 2").unwrap(); + fs::write(dir.path().join("c.txt"), "not ruby").unwrap(); + let files = collect_ruby_files(dir.path()); + assert_eq!(files.len(), 2); + } + + #[test] + fn collect_ruby_files_nested() { + let dir = TempDir::new().unwrap(); + let sub = dir.path().join("sub"); + fs::create_dir(&sub).unwrap(); + fs::write(sub.join("deep.rb"), "z = 3").unwrap(); + let files = collect_ruby_files(dir.path()); + assert_eq!(files.len(), 1); + } + + #[test] + fn collect_ruby_files_no_rb() { + let dir = TempDir::new().unwrap(); + fs::write(dir.path().join("readme.md"), "hello").unwrap(); + let files = collect_ruby_files(dir.path()); + assert!(files.is_empty()); + } + + #[test] + fn is_excluded_matching() { + let dir = TempDir::new().unwrap(); + let file = dir.path().join("test.rb"); + fs::write(&file, "x = 1").unwrap(); + let canonical = file.canonicalize().unwrap(); + let excluded: HashSet = [canonical].into_iter().collect(); + assert!(is_excluded(&file, &excluded)); + } + + #[test] + fn is_excluded_not_matching() { + let dir = TempDir::new().unwrap(); + let file = dir.path().join("test.rb"); + fs::write(&file, "x = 1").unwrap(); + let excluded: HashSet = HashSet::new(); + assert!(!is_excluded(&file, &excluded)); + } + + #[test] + fn collect_excluded_files_with_pattern() { + let dir = TempDir::new().unwrap(); + fs::write(dir.path().join("vendor.rb"), "x").unwrap(); + let patterns = vec![format!("{}/*.rb", dir.path().display())]; + let excluded = collect_excluded_files(&patterns, dir.path()); + assert!(!excluded.is_empty()); + } + + #[test] + fn collect_excluded_files_relative_pattern() { + let dir = TempDir::new().unwrap(); + fs::write(dir.path().join("skip.rb"), "x").unwrap(); + let patterns = vec!["*.rb".to_string()]; + let excluded = collect_excluded_files(&patterns, dir.path()); + assert!(!excluded.is_empty()); + } + + #[test] + fn collect_excluded_files_invalid_pattern() { + let excluded = collect_excluded_files(&["[invalid".to_string()], Path::new(".")); + assert!(excluded.is_empty()); + } + + #[test] + fn traverse_and_analyze_with_tempdir() { + let dir = TempDir::new().unwrap(); + fs::write(dir.path().join("test.rb"), "for x in [1]; end").unwrap(); + let config = Config::default(); + let result = traverse_and_analyze(dir.path(), &config); + assert_eq!(result.files_inspected, 1); + assert!(result.has_offenses()); + } + + #[test] + fn traverse_and_analyze_clean_file() { + let dir = TempDir::new().unwrap(); + fs::write(dir.path().join("clean.rb"), "x = 1 + 2").unwrap(); + let config = Config::default(); + let result = traverse_and_analyze(dir.path(), &config); + assert_eq!(result.files_inspected, 1); + assert!(!result.has_offenses()); + } + + #[test] + fn traverse_and_analyze_with_exclusion() { + let dir = TempDir::new().unwrap(); + fs::write(dir.path().join("test.rb"), "for x in [1]; end").unwrap(); + let config = Config::parse_yaml(&format!( + "exclude_paths:\n - '{}/*.rb'\n", + dir.path().display() + )) + .unwrap(); + let result = traverse_and_analyze(dir.path(), &config); + assert_eq!(result.files_inspected, 0); + } + + #[test] + fn traverse_and_analyze_parse_error() { + let dir = TempDir::new().unwrap(); + fs::write(dir.path().join("bad.rb"), "def def def").unwrap(); + let config = Config::default(); + let result = traverse_and_analyze(dir.path(), &config); + assert_eq!(result.files_inspected, 1); + // "def def def" produces a fatal parse error with no recoverable AST + assert_eq!(result.parse_errors.len(), 1); + assert!(result.results.is_empty()); + } +} diff --git a/src/fix.rs b/src/fix.rs index 8804328..164d04d 100644 --- a/src/fix.rs +++ b/src/fix.rs @@ -165,4 +165,57 @@ mod tests { let result = apply_fixes(source, &[fix]); assert_eq!(result, b"arr.flat_map { |x| [x] }"); } + + #[test] + fn apply_fixes_empty_fixes() { + let source = b"hello world"; + let result = apply_fixes(source, &[]); + assert_eq!(result, source); + } + + #[test] + fn apply_fixes_out_of_bounds_skipped() { + let source = b"short"; + let fix = Fix::single(10, 20, "big"); + let result = apply_fixes(source, &[fix]); + assert_eq!(result, b"short"); + } + + #[test] + fn apply_fixes_to_file_no_fixes() { + let dir = tempfile::TempDir::new().unwrap(); + let file = dir.path().join("test.rb"); + std::fs::write(&file, "x = 1").unwrap(); + let result = apply_fixes_to_file(&file, &[]).unwrap(); + assert_eq!(result, 0); + } + + #[test] + fn apply_fixes_to_file_valid_fix() { + let dir = tempfile::TempDir::new().unwrap(); + let file = dir.path().join("test.rb"); + std::fs::write(&file, "for x in [1]; end").unwrap(); + // Replace "for x in [1]; " with "[1].each do |x|; " + let fix = Fix::single(0, 14, "[1].each do |x|;"); + let result = apply_fixes_to_file(&file, &[fix]).unwrap(); + assert_eq!(result, 1); + } + + #[test] + fn apply_fixes_to_file_syntax_error_skipped() { + let dir = tempfile::TempDir::new().unwrap(); + let file = dir.path().join("test.rb"); + std::fs::write(&file, "x = 1 + 2").unwrap(); + // This fix produces invalid syntax + let fix = Fix::single(0, 9, "def def def"); + let result = apply_fixes_to_file(&file, &[fix]); + assert!(result.is_err()); + } + + #[test] + fn apply_fixes_to_file_nonexistent_file() { + let fix = Fix::single(0, 3, "x"); + let result = apply_fixes_to_file(Path::new("/nonexistent.rb"), &[fix]); + assert!(result.is_err()); + } } diff --git a/src/offense.rs b/src/offense.rs index 8626c0c..8197cf3 100644 --- a/src/offense.rs +++ b/src/offense.rs @@ -158,3 +158,59 @@ impl Offense { } } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn all_returns_19_variants() { + assert_eq!(OffenseKind::all().len(), 19); + } + + #[test] + fn config_key_roundtrips_via_from_config_key() { + for kind in OffenseKind::all() { + let key = kind.config_key(); + let restored = OffenseKind::from_config_key(key); + assert_eq!(restored, Some(*kind), "roundtrip failed for {}", key); + } + } + + #[test] + fn from_config_key_returns_none_for_unknown() { + assert_eq!(OffenseKind::from_config_key("nonexistent"), None); + } + + #[test] + fn explanation_is_non_empty_for_all() { + for kind in OffenseKind::all() { + let explanation = kind.explanation(); + assert!(!explanation.is_empty(), "empty explanation for {:?}", kind); + } + } + + #[test] + fn display_matches_config_key() { + for kind in OffenseKind::all() { + assert_eq!(format!("{}", kind), kind.config_key()); + } + } + + #[test] + fn offense_new_has_no_fix() { + let offense = Offense::new(OffenseKind::GsubVsTr, 42); + assert_eq!(offense.kind, OffenseKind::GsubVsTr); + assert_eq!(offense.line, 42); + assert!(offense.fix.is_none()); + } + + #[test] + fn offense_with_fix_has_fix() { + let fix = Fix::single(0, 5, "hello"); + let offense = Offense::with_fix(OffenseKind::ForLoopVsEach, 10, fix); + assert_eq!(offense.kind, OffenseKind::ForLoopVsEach); + assert_eq!(offense.line, 10); + assert!(offense.fix.is_some()); + } +} diff --git a/src/output.rs b/src/output.rs index 98f08d6..2d3958f 100644 --- a/src/output.rs +++ b/src/output.rs @@ -223,7 +223,7 @@ fn print_fix_statistics(result: &TraversalResult, total_fixed: usize, total_erro fixed_str.to_string() }; - let unfixable = offenses - fixable; + let unfixable = offenses.saturating_sub(fixable); if total_errors > 0 { let err_str = format!( "{} {} skipped (syntax error after fix)", @@ -267,3 +267,176 @@ fn pluralize(word: &str, count: usize) -> String { format!("{}s", word) } } + +#[cfg(test)] +mod tests { + use super::*; + use crate::analyzer::AnalysisResult; + use crate::fix::Fix; + use crate::offense::{Offense, OffenseKind}; + + #[test] + fn pluralize_singular() { + assert_eq!(pluralize("file", 1), "file"); + } + + #[test] + fn pluralize_plural() { + assert_eq!(pluralize("file", 0), "files"); + assert_eq!(pluralize("offense", 2), "offenses"); + } + + fn make_result(offenses: Vec) -> TraversalResult { + TraversalResult { + results: vec![AnalysisResult { + path: "test.rb".to_string(), + offenses, + }], + parse_errors: vec![], + files_inspected: 1, + } + } + + #[test] + fn filter_unfixable_keeps_only_no_fix() { + let offenses = vec![ + Offense::new(OffenseKind::GsubVsTr, 1), + Offense::with_fix(OffenseKind::ForLoopVsEach, 2, Fix::single(0, 3, "x")), + Offense::new(OffenseKind::SortVsSortBy, 3), + ]; + let result = make_result(offenses); + let filtered = filter_unfixable(&result); + assert_eq!(filtered.results[0].offenses.len(), 2); + assert!(filtered.results[0].offenses.iter().all(|o| o.fix.is_none())); + } + + #[test] + fn filter_unfixable_empty_when_all_fixable() { + let offenses = vec![Offense::with_fix( + OffenseKind::ForLoopVsEach, + 1, + Fix::single(0, 3, "x"), + )]; + let result = make_result(offenses); + let filtered = filter_unfixable(&result); + assert_eq!(filtered.results[0].offenses.len(), 0); + } + + #[test] + fn print_results_by_file_no_panic() { + let result = make_result(vec![Offense::new(OffenseKind::GsubVsTr, 5)]); + print_results_by_file(&result); + } + + #[test] + fn print_results_by_file_empty_no_panic() { + let result = make_result(vec![]); + print_results_by_file(&result); + } + + #[test] + fn print_results_by_rule_no_panic() { + let result = make_result(vec![ + Offense::new(OffenseKind::GsubVsTr, 5), + Offense::new(OffenseKind::GsubVsTr, 10), + ]); + print_results_by_rule(&result); + } + + #[test] + fn print_results_plain_no_panic() { + let result = make_result(vec![Offense::new(OffenseKind::GsubVsTr, 5)]); + print_results_plain(&result); + } + + #[test] + fn print_results_plain_empty_no_panic() { + let result = make_result(vec![]); + print_results_plain(&result); + } + + #[test] + fn print_statistics_no_offenses() { + let result = make_result(vec![]); + print_statistics(&result); + } + + #[test] + fn print_statistics_with_offenses() { + let result = make_result(vec![Offense::new(OffenseKind::GsubVsTr, 5)]); + print_statistics(&result); + } + + #[test] + fn print_statistics_with_parse_errors() { + let result = TraversalResult { + results: vec![], + parse_errors: vec![ParseError { + path: "bad.rb".to_string(), + message: "syntax error".to_string(), + }], + files_inspected: 1, + }; + print_statistics(&result); + } + + #[test] + fn print_parse_errors_no_panic() { + let errors = vec![ParseError { + path: "bad.rb".to_string(), + message: "oops".to_string(), + }]; + print_parse_errors(&errors); + } + + #[test] + fn print_results_dispatches_all_formats() { + let result = make_result(vec![Offense::new(OffenseKind::GsubVsTr, 1)]); + print_results(&result, &OutputFormat::File); + print_results(&result, &OutputFormat::Rule); + print_results(&result, &OutputFormat::Plain); + } + + #[test] + fn print_fix_results_no_panic() { + let offenses = vec![ + Offense::new(OffenseKind::GsubVsTr, 1), + Offense::with_fix(OffenseKind::ForLoopVsEach, 2, Fix::single(0, 3, "x")), + ]; + let result = make_result(offenses); + print_fix_results(&result, 1, 0, &OutputFormat::File); + } + + #[test] + fn print_fix_results_with_errors() { + let offenses = vec![Offense::with_fix( + OffenseKind::ForLoopVsEach, + 1, + Fix::single(0, 3, "x"), + )]; + let result = make_result(offenses); + print_fix_results(&result, 0, 1, &OutputFormat::File); + } + + #[test] + fn print_fix_results_all_fixed() { + let offenses = vec![Offense::with_fix( + OffenseKind::ForLoopVsEach, + 1, + Fix::single(0, 3, "x"), + )]; + let result = make_result(offenses); + print_fix_results(&result, 1, 0, &OutputFormat::File); + } + + #[test] + fn print_fix_results_unfixable_remaining() { + let offenses = vec![ + Offense::new(OffenseKind::GsubVsTr, 1), + Offense::with_fix(OffenseKind::ForLoopVsEach, 2, Fix::single(0, 3, "x")), + ]; + let result = make_result(offenses); + print_fix_results(&result, 1, 0, &OutputFormat::Rule); + print_fix_results(&result, 1, 0, &OutputFormat::Plain); + } +} diff --git a/src/scanner/for_loop_scanner.rs b/src/scanner/for_loop_scanner.rs index b93a12c..1974bc8 100644 --- a/src/scanner/for_loop_scanner.rs +++ b/src/scanner/for_loop_scanner.rs @@ -129,4 +129,41 @@ mod tests { panic!("Expected For node"); } } + + #[test] + fn extract_trimmed_valid() { + let source = b" hello "; + let result = extract_trimmed(source, 0, 9); + assert_eq!(result, Some("hello".to_string())); + } + + #[test] + fn extract_trimmed_start_ge_end() { + assert_eq!(extract_trimmed(b"hello", 5, 3), None); + assert_eq!(extract_trimmed(b"hello", 3, 3), None); + } + + #[test] + fn extract_trimmed_end_gt_len() { + assert_eq!(extract_trimmed(b"hi", 0, 10), None); + } + + #[test] + fn extract_trimmed_empty_after_trim() { + let result = extract_trimmed(b" ", 0, 3); + assert_eq!(result, Some("".to_string())); + } + + #[test] + fn scan_always_returns_offense() { + // Even when fix fails, we should still get an offense + let source = b"for x in arr; end"; + let result = lib_ruby_parser::Parser::new(source.to_vec(), Default::default()).do_parse(); + let ast = result.ast.unwrap(); + if let lib_ruby_parser::Node::For(f) = ast.as_ref() { + let offenses = scan(f, source); + assert_eq!(offenses.len(), 1); + assert_eq!(offenses[0].kind, OffenseKind::ForLoopVsEach); + } + } } diff --git a/src/scanner/method_call_scanner.rs b/src/scanner/method_call_scanner.rs index a8db35a..8b79072 100644 --- a/src/scanner/method_call_scanner.rs +++ b/src/scanner/method_call_scanner.rs @@ -608,4 +608,199 @@ mod tests { let o = parse_and_collect(b"->(x) { x.to_s }"); assert!(!o.iter().any(|x| x.kind == OffenseKind::BlockVsSymbolToProc)); } + + // --- Additional edge case tests --- + + #[test] + fn first_not_on_shuffle_no_fire() { + let o = parse_and_collect(b"arr.first"); + assert!(!o + .iter() + .any(|x| x.kind == OffenseKind::ShuffleFirstVsSample)); + } + + #[test] + fn reverse_not_each_no_fire() { + let o = parse_and_collect(b"arr.reverse.map { |x| x }"); + assert!(!o + .iter() + .any(|x| x.kind == OffenseKind::ReverseEachVsReverseEach)); + } + + #[test] + fn select_first_with_block_pass() { + let o = parse_and_collect(b"arr.select(&:odd?).first"); + assert!(o.iter().any(|x| x.kind == OffenseKind::SelectFirstVsDetect)); + } + + #[test] + fn select_last_with_block_pass() { + let o = parse_and_collect(b"arr.select(&:odd?).last"); + assert!(o + .iter() + .any(|x| x.kind == OffenseKind::SelectLastVsReverseDetect)); + } + + #[test] + fn map_flatten_with_arg_2_no_fire() { + let o = parse_and_collect(b"arr.map { |e| [e] }.flatten(2)"); + assert!(!o.iter().any(|x| x.kind == OffenseKind::MapFlattenVsFlatMap)); + } + + #[test] + fn select_first_with_args_no_fire() { + let o = parse_and_collect(b"arr.select { |x| x > 1 }.first(3)"); + assert!(!o.iter().any(|x| x.kind == OffenseKind::SelectFirstVsDetect)); + } + + #[test] + fn select_last_with_args_no_fire() { + let o = parse_and_collect(b"arr.select { |x| x > 1 }.last(3)"); + assert!(!o + .iter() + .any(|x| x.kind == OffenseKind::SelectLastVsReverseDetect)); + } + + #[test] + fn module_eval_with_def_string() { + let o = parse_and_collect(b"klass.module_eval(\"def foo; end\")"); + assert!(o.iter().any(|x| x.kind == OffenseKind::ModuleEval)); + } + + #[test] + fn module_eval_without_def_no_fire() { + let o = parse_and_collect(b"klass.module_eval(\"puts 1\")"); + assert!(!o.iter().any(|x| x.kind == OffenseKind::ModuleEval)); + } + + #[test] + fn module_eval_non_string_no_fire() { + let o = parse_and_collect(b"klass.module_eval(some_var)"); + assert!(!o.iter().any(|x| x.kind == OffenseKind::ModuleEval)); + } + + #[test] + fn module_eval_with_block() { + let o = parse_and_collect(b"klass.module_eval { define_method(:foo) {} }"); + assert!(!o.iter().any(|x| x.kind == OffenseKind::ModuleEval)); + } + + #[test] + fn block_multiple_args_no_symbol_to_proc() { + let o = parse_and_collect(b"arr.each_with_object([]) { |x, acc| x.to_s }"); + assert!(!o.iter().any(|x| x.kind == OffenseKind::BlockVsSymbolToProc)); + } + + #[test] + fn block_no_body_no_symbol_to_proc() { + // Empty block body + let o = parse_and_collect(b"arr.map { |x| }"); + assert!(!o.iter().any(|x| x.kind == OffenseKind::BlockVsSymbolToProc)); + } + + #[test] + fn block_receiver_not_lvar_no_symbol_to_proc() { + let o = parse_and_collect(b"arr.map { |x| @y.to_s }"); + assert!(!o.iter().any(|x| x.kind == OffenseKind::BlockVsSymbolToProc)); + } + + #[test] + fn block_receiver_is_primitive_no_symbol_to_proc() { + let o = parse_and_collect(b"arr.map { |x| 42.to_s }"); + assert!(!o.iter().any(|x| x.kind == OffenseKind::BlockVsSymbolToProc)); + } + + #[test] + fn hash_merge_bang_no_args_no_fire() { + let o = parse_and_collect(b"h.merge!"); + assert!(!o + .iter() + .any(|x| x.kind == OffenseKind::HashMergeBangVsHashBrackets)); + } + + #[test] + fn gsub_one_arg_no_fire() { + let o = parse_and_collect(b"s.gsub('x')"); + assert!(!o.iter().any(|x| x.kind == OffenseKind::GsubVsTr)); + } + + #[test] + fn fetch_one_arg_no_fire() { + let o = parse_and_collect(b"h.fetch(:key)"); + assert!(!o + .iter() + .any(|x| x.kind == OffenseKind::FetchWithArgumentVsBlock)); + } + + #[test] + fn include_not_on_range_no_fire() { + let o = parse_and_collect(b"[1,2,3].include?(5)"); + assert!(!o + .iter() + .any(|x| x.kind == OffenseKind::IncludeVsCoverOnRange)); + } + + #[test] + fn include_on_exclusive_range() { + let o = parse_and_collect(b"(1...10).include?(5)"); + assert!(o + .iter() + .any(|x| x.kind == OffenseKind::IncludeVsCoverOnRange)); + } + + #[test] + fn include_on_parenthesized_range() { + // Single-paren range — parsed as Begin(Irange) + let o = parse_and_collect(b"(1..10).include?(5)"); + assert!(o + .iter() + .any(|x| x.kind == OffenseKind::IncludeVsCoverOnRange)); + } + + #[test] + fn sort_without_block_no_fire() { + let o = parse_and_collect(b"arr.sort"); + assert!(!o.iter().any(|x| x.kind == OffenseKind::SortVsSortBy)); + } + + #[test] + fn block_wrong_lvar_name_no_symbol_to_proc() { + let o = parse_and_collect(b"arr.map { |x| y.to_s }"); + assert!(!o.iter().any(|x| x.kind == OffenseKind::BlockVsSymbolToProc)); + } + + #[test] + fn block_with_args_on_outer_no_symbol_to_proc() { + let o = parse_and_collect(b"arr.inject(0) { |x| x.to_s }"); + assert!(!o.iter().any(|x| x.kind == OffenseKind::BlockVsSymbolToProc)); + } + + #[test] + fn module_eval_with_heredoc_containing_def() { + let o = parse_and_collect(b"klass.module_eval(<<~RUBY)\n def foo\n 42\n end\nRUBY\n"); + assert!(o.iter().any(|x| x.kind == OffenseKind::ModuleEval)); + } + + #[test] + fn keys_each_with_keys_having_args_no_fire() { + // keys("x") is not Hash#keys — should not fire + let o = parse_and_collect(b"h.keys(\"x\").each { |k| k }"); + assert!(!o.iter().any(|x| x.kind == OffenseKind::KeysEachVsEachKey)); + } + + #[test] + fn each_with_index_without_block_still_fires() { + let o = parse_and_collect(b"arr.each_with_index"); + assert!(o + .iter() + .any(|x| x.kind == OffenseKind::EachWithIndexVsWhile)); + } + + #[test] + fn fetch_with_block_pass_no_fire() { + let o = parse_and_collect(b"h.fetch(:key, &block)"); + assert!(!o + .iter() + .any(|x| x.kind == OffenseKind::FetchWithArgumentVsBlock)); + } } diff --git a/src/scanner/method_definition_scanner.rs b/src/scanner/method_definition_scanner.rs index 9b1c1ee..bd8d2d5 100644 --- a/src/scanner/method_definition_scanner.rs +++ b/src/scanner/method_definition_scanner.rs @@ -184,4 +184,93 @@ mod tests { .iter() .any(|o| o.kind == OffenseKind::ProcCallVsYield)); } + + #[test] + fn setter_wrong_ivar_name_no_fire() { + let offenses = parse_and_scan(b"def name=(v); @other = v; end"); + assert!(!offenses + .iter() + .any(|o| o.kind == OffenseKind::SetterVsAttrWriter)); + } + + #[test] + fn setter_wrong_value_no_fire() { + let offenses = parse_and_scan(b"def name=(v); @name = 42; end"); + assert!(!offenses + .iter() + .any(|o| o.kind == OffenseKind::SetterVsAttrWriter)); + } + + #[test] + fn setter_multiple_args_no_fire() { + let offenses = parse_and_scan(b"def name=(a, b); @name = a; end"); + assert!(!offenses + .iter() + .any(|o| o.kind == OffenseKind::SetterVsAttrWriter)); + } + + #[test] + fn setter_no_body_no_fire() { + let offenses = parse_and_scan(b"def name=(v); end"); + assert!(!offenses + .iter() + .any(|o| o.kind == OffenseKind::SetterVsAttrWriter)); + } + + #[test] + fn getter_with_args_no_fire() { + let offenses = parse_and_scan(b"def name(x); @name; end"); + assert!(!offenses + .iter() + .any(|o| o.kind == OffenseKind::GetterVsAttrReader)); + } + + #[test] + fn getter_multiple_body_stmts_no_fire() { + let offenses = parse_and_scan(b"def name; puts 'x'; @name; end"); + assert!(!offenses + .iter() + .any(|o| o.kind == OffenseKind::GetterVsAttrReader)); + } + + #[test] + fn getter_wrong_ivar_no_fire() { + let offenses = parse_and_scan(b"def name; @other; end"); + assert!(!offenses + .iter() + .any(|o| o.kind == OffenseKind::GetterVsAttrReader)); + } + + #[test] + fn getter_no_body_no_fire() { + let offenses = parse_and_scan(b"def name; end"); + assert!(!offenses + .iter() + .any(|o| o.kind == OffenseKind::GetterVsAttrReader)); + } + + #[test] + fn proc_call_nested_in_body() { + let offenses = parse_and_scan(b"def foo(&block); if true; block.call; end; end"); + assert!(offenses + .iter() + .any(|o| o.kind == OffenseKind::ProcCallVsYield)); + } + + #[test] + fn setter_name_method_is_not_getter() { + // name= should not trigger getter check + let offenses = parse_and_scan(b"def name=(v); @name = v; end"); + assert!(!offenses + .iter() + .any(|o| o.kind == OffenseKind::GetterVsAttrReader)); + } + + #[test] + fn setter_body_not_ivasgn_no_fire() { + let offenses = parse_and_scan(b"def name=(v); puts v; end"); + assert!(!offenses + .iter() + .any(|o| o.kind == OffenseKind::SetterVsAttrWriter)); + } } diff --git a/src/scanner/rescue_scanner.rs b/src/scanner/rescue_scanner.rs index 577545d..0004322 100644 --- a/src/scanner/rescue_scanner.rs +++ b/src/scanner/rescue_scanner.rs @@ -75,4 +75,24 @@ mod tests { let offenses = parse_and_find_rescue_bodies(b"begin; rescue; end"); assert_eq!(offenses.len(), 0); } + + #[test] + fn multiple_exceptions_including_no_method_error() { + let offenses = + parse_and_find_rescue_bodies(b"begin; rescue ArgumentError, NoMethodError; end"); + assert_eq!(offenses.len(), 1); + } + + #[test] + fn scoped_no_method_error_no_fire() { + let offenses = + parse_and_find_rescue_bodies(b"begin; rescue SomeModule::NoMethodError; end"); + assert_eq!(offenses.len(), 0); + } + + #[test] + fn rescue_other_error_no_fire() { + let offenses = parse_and_find_rescue_bodies(b"begin; rescue TypeError; end"); + assert_eq!(offenses.len(), 0); + } } diff --git a/tests/fixtures/14b_module_eval_heredoc.rb b/tests/fixtures/14b_module_eval_heredoc.rb new file mode 100644 index 0000000..258058c --- /dev/null +++ b/tests/fixtures/14b_module_eval_heredoc.rb @@ -0,0 +1,6 @@ +# Module eval with heredoc containing def — should fire +klass.module_eval(<<~RUBY) + def hello + "world" + end +RUBY diff --git a/tests/integration/cli_tests.rs b/tests/integration/cli_tests.rs index 448ab1e..af0fda1 100644 --- a/tests/integration/cli_tests.rs +++ b/tests/integration/cli_tests.rs @@ -79,3 +79,92 @@ fn statistics_line_present() { stdout ); } + +#[test] +fn format_rule_output() { + let output = cargo_bin() + .args(["tests/fixtures/19_for_loop.rb", "--format", "rule"]) + .output() + .expect("Failed to run"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("offense"), + "Expected offense count in rule output: {}", + stdout + ); +} + +#[test] +fn format_plain_output() { + let output = cargo_bin() + .args(["tests/fixtures/19_for_loop.rb", "--format", "plain"]) + .output() + .expect("Failed to run"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("19_for_loop.rb:"), + "Expected path:line format in plain output: {}", + stdout + ); +} + +#[test] +fn fix_mode_modifies_file() { + let dir = tempfile::TempDir::new().unwrap(); + let file = dir.path().join("fixable.rb"); + std::fs::write(&file, "for x in [1,2,3]; puts x; end\n").unwrap(); + let output = cargo_bin() + .args([file.to_str().unwrap(), "--fix"]) + .output() + .expect("Failed to run"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("fixed"), + "Expected 'fixed' in fix output: {}", + stdout + ); + let content = std::fs::read_to_string(&file).unwrap(); + assert!( + content.contains(".each do"), + "Expected file to be fixed: {}", + content + ); +} + +#[test] +fn fix_mode_reports_unfixable() { + let dir = tempfile::TempDir::new().unwrap(); + let file = dir.path().join("unfixable.rb"); + // sort with block is unfixable + std::fs::write(&file, "arr.sort { |a, b| a <=> b }\n").unwrap(); + let output = cargo_bin() + .args([file.to_str().unwrap(), "--fix"]) + .output() + .expect("Failed to run"); + let stdout = String::from_utf8_lossy(&output.stdout); + assert!( + stdout.contains("cannot be auto-fixed") || stdout.contains("0 offenses fixed"), + "Expected unfixable note in output: {}", + stdout + ); +} + +#[test] +fn config_disables_rule() { + let dir = tempfile::TempDir::new().unwrap(); + std::fs::write(dir.path().join("test.rb"), "for x in [1]; end\n").unwrap(); + std::fs::write( + dir.path().join(".rubyfast.yml"), + "speedups:\n for_loop_vs_each: false\n", + ) + .unwrap(); + let output = cargo_bin() + .arg(dir.path().to_str().unwrap()) + .output() + .expect("Failed to run"); + assert!( + output.status.success(), + "Expected exit 0 when rule is disabled. stdout: {}", + String::from_utf8_lossy(&output.stdout) + ); +} diff --git a/tests/integration/fixture_tests.rs b/tests/integration/fixture_tests.rs index 8ce1ca5..c005a05 100644 --- a/tests/integration/fixture_tests.rs +++ b/tests/integration/fixture_tests.rs @@ -147,3 +147,9 @@ fn clean_file_has_no_offenses() { let kinds = analyze("clean.rb"); assert!(kinds.is_empty(), "Expected no offenses, got: {:?}", kinds); } + +#[test] +fn module_eval_heredoc() { + let kinds = analyze("14b_module_eval_heredoc.rb"); + assert_eq!(count_kind(&kinds, OffenseKind::ModuleEval), 1); +}