Skip to content

Commit

Permalink
[tests] improve issue type suppression unit tests
Browse files Browse the repository at this point in the history 
Summary:
Change the unit tests to be expect tests instead of having to buid
suppression maps by hand to compare to. Bonus: we also record which
parse errors are expected, and avoid spamming stderr when running unit
tests.

Reviewed By: ngorogiannis

Differential Revision:
D66700356

Privacy Context Container: L1208441

fbshipit-source-id: 49a4c6731f3ac9b54f2cc5c5fad9302c46323d8f
  • Loading branch information
@jvillard @facebook-github-bot
jvillard authored and facebook-github-bot committed Dec 3, 2024
1 parent d3dce16 commit 68f7ede
Show file tree
Hide file tree
Showing 4 changed files with 126 additions and 91 deletions.
5 changes: 4 additions & 1 deletion infer/src/integration/JsonReports.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -241,7 +241,10 @@ module JsonIssuePrinter = MakeJsonListPrinter (struct
L.user_error "Could not read file %s@\n" filename ;
IString.Map.empty
| Ok lines ->
Suppressions.parse_lines ~file:filename lines )
let suppressions, errors = Suppressions.parse_lines ~file:filename lines in
List.iter errors ~f:(fun (Suppressions.UserError error) ->
L.user_error "%s" (error ()) ) ;
suppressions )
in
suppressions_cache := SourceFile.Map.add source_file suppressions !suppressions_cache ;
Suppressions.is_suppressed ~suppressions ~issue_type ~line
Expand Down
8 changes: 2 additions & 6 deletions infer/src/integration/Suppressions.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ module ParseResult = struct

type 'a parse_result = 'a * error list

let _pp_parse_result pp fmt ((x, errors) : _ parse_result) =
let pp_parse_result pp fmt ((x, errors) : _ parse_result) =
if List.is_empty errors then pp fmt x
else
F.fprintf fmt "@[RESULT: @[%a@]@\nERRORS: @[%a@]@]" pp x
Expand Down Expand Up @@ -224,11 +224,7 @@ let parse_lines ?file lines =
res


let parse_lines ?file lines =
let suppressions, errors = parse_lines ?file lines in
List.iter errors ~f:(fun (UserError error) -> L.user_error "%s" (error ())) ;
suppressions

let pp_parse_result fmt parse_result = pp_parse_result pp fmt parse_result

let first_key_match ~suppressions s =
IString.Map.to_seq suppressions
Expand Down
8 changes: 7 additions & 1 deletion infer/src/integration/Suppressions.mli
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
*)

open! IStd
module F = Format

module Span : sig
type block = {first: int; last: int} [@@deriving compare, equal]
Expand All @@ -15,6 +16,11 @@ end

type t = Span.t IString.Map.t

val parse_lines : ?file:string -> string list -> t
type error = UserError of (unit -> string)

val parse_lines : ?file:string -> string list -> t * error list

val is_suppressed : suppressions:t -> issue_type:string -> line:int -> bool

val pp_parse_result : F.formatter -> t * error list -> unit [@@warning "-unused-value-declaration"]
(* used in unit tests *)
196 changes: 113 additions & 83 deletions infer/src/integration/unit/SuppressionsTest.ml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -6,136 +6,164 @@
*)

open! IStd
module Map = IString.Map

(** {2 parsing} *)

let%test "parsing empty file" = Map.is_empty @@ Suppressions.parse_lines []
let t parse_result = Format.printf "%a" Suppressions.pp_parse_result parse_result

let%test "parsing empty string" = Map.is_empty @@ Suppressions.parse_lines [""]
let%expect_test "parsing empty file" =
t @@ Suppressions.parse_lines [] ;
[%expect {| Empty |}]

let%test "parsing non-matching line" = Map.is_empty @@ Suppressions.parse_lines ["1+1 #hello"]

let%test "parsing matching line" =
Map.equal Suppressions.Span.equal
(Suppressions.parse_lines ["1+1 // @infer-ignore BUFFER_OVERRUN_L1"])
(Map.singleton "BUFFER_OVERRUN_L1" @@ Suppressions.Span.Blocks [{first= 1; last= 2}])
let%expect_test "parsing empty string" =
t @@ Suppressions.parse_lines [""] ;
[%expect {| Empty |}]


let%test "parsing matching line inside string gotcha" =
Map.equal Suppressions.Span.equal
(Suppressions.parse_lines ["const char* s = \"@infer-ignore BUFFER_OVERRUN_L1,\";"])
(Map.singleton "BUFFER_OVERRUN_L1" @@ Suppressions.Span.Blocks [{first= 1; last= 2}])
let%expect_test "parsing non-matching line" =
t @@ Suppressions.parse_lines ["1+1 #hello"] ;
[%expect {| Empty |}]


let%test "parsing matching line no issue type" =
Map.is_empty @@ Suppressions.parse_lines ["1+1 // @infer-ignore "]
let%expect_test "parsing matching line" =
t @@ Suppressions.parse_lines ["1+1 // @infer-ignore BUFFER_OVERRUN_L1"] ;
[%expect {| BUFFER_OVERRUN_L1: Blocks 1-2 |}]


let%test "parsing half-matching line no issue type" =
Map.is_empty @@ Suppressions.parse_lines ["1+1 // @infer-ignore-all"]
let%expect_test "parsing matching line inside string gotcha" =
t @@ Suppressions.parse_lines ["const char* s = \"@infer-ignore BUFFER_OVERRUN_L1,\";"] ;
[%expect
{|
RESULT: BUFFER_OVERRUN_L1: Blocks 1-2

ERRORS: "; not a valid issue_type / wildcard |}]
let%test "parsing matching line multiple issue types" =
Map.equal Suppressions.Span.equal
(Suppressions.parse_lines ["1+1 // @infer-ignore BUFFER_OVERRUN_L1,PULSE_UNNECESSARY_COPY"])
Map.(
empty
|> add "BUFFER_OVERRUN_L1" (Suppressions.Span.Blocks [{first= 1; last= 2}])
|> add "PULSE_UNNECESSARY_COPY" (Suppressions.Span.Blocks [{first= 1; last= 2}]) )
let%expect_test "parsing matching line no issue type" =
t @@ Suppressions.parse_lines ["1+1 // @infer-ignore "] ;
[%expect {| Empty |}]
let%test "parsing matching line multiple noise" =
Map.equal Suppressions.Span.equal
(Suppressions.parse_lines
["1+1 // @infer-ignore BUFFER_OVERRUN_L1,,,, PULSE_UNNECESSARY_COPY,,,,,,,"] )
Map.(
empty
|> add "BUFFER_OVERRUN_L1" (Suppressions.Span.Blocks [{first= 1; last= 2}])
|> add "PULSE_UNNECESSARY_COPY" (Suppressions.Span.Blocks [{first= 1; last= 2}]) )
let%expect_test "parsing half-matching line no issue type" =
t @@ Suppressions.parse_lines ["1+1 // @infer-ignore-all"] ;
[%expect {| Empty |}]
let%test "multi line block" =
Map.equal Suppressions.Span.equal
(Suppressions.parse_lines
["// @infer-ignore BUFFER_OVERRUN_L1"; "1+1 // @infer-ignore ,PULSE_UNNECESSARY_COPY"] )
Map.(
empty
|> add "BUFFER_OVERRUN_L1" (Suppressions.Span.Blocks [{first= 1; last= 3}])
|> add "PULSE_UNNECESSARY_COPY" (Suppressions.Span.Blocks [{first= 1; last= 3}]) )
let%expect_test "parsing matching line multiple issue types" =
t @@ Suppressions.parse_lines ["1+1 // @infer-ignore BUFFER_OVERRUN_L1,PULSE_UNNECESSARY_COPY"] ;
[%expect {|
BUFFER_OVERRUN_L1: Blocks 1-2
PULSE_UNNECESSARY_COPY: Blocks 1-2 |}]
let%test "multiple blocks" =
Map.equal Suppressions.Span.equal
(Suppressions.parse_lines
["// @infer-ignore BUFFER_OVERRUN_L1"; ""; "1+1 // @infer-ignore BUFFER_OVERRUN_L1"] )
Map.(
empty
|> add "BUFFER_OVERRUN_L1"
(Suppressions.Span.Blocks [{first= 1; last= 2}; {first= 3; last= 4}]) )
let%expect_test "parsing matching line multiple noise" =
t
@@ Suppressions.parse_lines
["1+1 // @infer-ignore BUFFER_OVERRUN_L1,,,, PULSE_UNNECESSARY_COPY,,,,,,,"] ;
[%expect {|
BUFFER_OVERRUN_L1: Blocks 1-2
PULSE_UNNECESSARY_COPY: Blocks 1-2 |}]
let%test "parsing matching line every" =
Map.equal Suppressions.Span.equal
(Suppressions.parse_lines ["1+1 // @infer-ignore-every BUFFER_OVERRUN_L1"])
(Map.singleton "BUFFER_OVERRUN_L1" @@ Suppressions.Span.Every)
let%expect_test "multi line block" =
t
@@ Suppressions.parse_lines
["// @infer-ignore BUFFER_OVERRUN_L1"; "1+1 // @infer-ignore ,PULSE_UNNECESSARY_COPY"] ;
[%expect {|
BUFFER_OVERRUN_L1: Blocks 1-3
PULSE_UNNECESSARY_COPY: Blocks 1-3 |}]
let%test "every overrides block" =
Map.equal Suppressions.Span.equal
(Suppressions.parse_lines
["//@infer-ignore BUFFER_OVERRUN_L1"; ""; "1+1 // @infer-ignore-every BUFFER_OVERRUN_L1"] )
(Map.singleton "BUFFER_OVERRUN_L1" @@ Suppressions.Span.Every)
let%expect_test "multiple blocks" =
t
@@ Suppressions.parse_lines
["// @infer-ignore BUFFER_OVERRUN_L1"; ""; "1+1 // @infer-ignore BUFFER_OVERRUN_L1"] ;
[%expect {| BUFFER_OVERRUN_L1: Blocks 1-2, 3-4 |}]
let%test "block doesn't override every" =
Map.equal Suppressions.Span.equal
(Suppressions.parse_lines
["//@infer-ignore-every BUFFER_OVERRUN_L1"; ""; "1+1 // @infer-ignore BUFFER_OVERRUN_L1"] )
(Map.singleton "BUFFER_OVERRUN_L1" @@ Suppressions.Span.Every)
let%expect_test "parsing matching line every" =
t @@ Suppressions.parse_lines ["1+1 // @infer-ignore-every BUFFER_OVERRUN_L1"] ;
[%expect {| BUFFER_OVERRUN_L1: Every |}]
let%test "both ignore and ignore-every on single line" =
Map.equal Suppressions.Span.equal
(Suppressions.parse_lines
let%expect_test "every overrides block" =
t
@@ Suppressions.parse_lines
["//@infer-ignore BUFFER_OVERRUN_L1"; ""; "1+1 // @infer-ignore-every BUFFER_OVERRUN_L1"] ;
[%expect {| BUFFER_OVERRUN_L1: Every |}]
let%expect_test "block doesn't override every" =
t
@@ Suppressions.parse_lines
["//@infer-ignore-every BUFFER_OVERRUN_L1"; ""; "1+1 // @infer-ignore BUFFER_OVERRUN_L1"] ;
[%expect {| BUFFER_OVERRUN_L1: Every |}]
let%expect_test "both ignore and ignore-every on single line" =
t
@@ Suppressions.parse_lines
[ "1+1 // @infer-ignore-every BUFFER_OVERRUN_L1,PULSE_UNNECESSARY_COPY @infer-ignore \
DEAD_STORE" ] )
(* PULSE_UNNECESSARY_COPY @infer-ignore DEAD_STORE not a valid issue_type / wildcard *)
(Map.singleton "BUFFER_OVERRUN_L1" @@ Suppressions.Span.Every)
DEAD_STORE" ] ;
[%expect
{|
RESULT: BUFFER_OVERRUN_L1: Every
ERRORS: PULSE_UNNECESSARY_COPY @infer-ignore DEAD_STORE not a valid issue_type / wildcard
;
Both @infer-ignore-every and @infer-ignore found in line 2 |}]
let%test "both ignore and ignore-every on single line every wins" =
Map.equal Suppressions.Span.equal
(Suppressions.parse_lines
let%expect_test "both ignore and ignore-every on single line every wins" =
t
@@ Suppressions.parse_lines
[ "1+1 // @infer-ignore BUFFER_OVERRUN_L1,PULSE_UNNECESSARY_COPY @infer-ignore-every \
DEAD_STORE" ] )
(Map.singleton "DEAD_STORE" @@ Suppressions.Span.Every)
DEAD_STORE" ] ;
[%expect
{|
RESULT: DEAD_STORE: Every
ERRORS: Both @infer-ignore-every and @infer-ignore found in line 2 |}]
let%test "simple wildcard" =
Map.equal Suppressions.Span.equal
(Suppressions.parse_lines ["1+1 // @infer-ignore-every PULSE_UNNECESSARY_.*"])
(Map.singleton "PULSE_UNNECESSARY_.*" @@ Suppressions.Span.Every)
let%expect_test "simple wildcard" =
t @@ Suppressions.parse_lines ["1+1 // @infer-ignore-every PULSE_UNNECESSARY_.*"] ;
[%expect {| PULSE_UNNECESSARY_.*: Every |}]
let%test "match everything wildcard is invalid" =
Map.is_empty @@ Suppressions.parse_lines ["1+1 // @infer-ignore-every .*"]
let%expect_test "match everything wildcard is invalid" =
t @@ Suppressions.parse_lines ["1+1 // @infer-ignore-every .*"] ;
[%expect {|
RESULT: Empty
ERRORS: .* not a valid issue_type / wildcard |}]
let%test "syntax error wildcard" =
Map.is_empty @@ Suppressions.parse_lines ["1+1 // @infer-ignore-every *"]
let%expect_test "syntax error wildcard" =
t @@ Suppressions.parse_lines ["1+1 // @infer-ignore-every *"] ;
[%expect
{|
RESULT: Empty
ERRORS: * not a valid issue_type / wildcard
; Invalid regex: * |}]
(** {2 matching} *)
let s1 = Suppressions.parse_lines ["1+1 // @infer-ignore BUFFER_OVERRUN_L1"]
let no_error (x, errors) =
assert (List.is_empty errors) ;
x
let s1 = Suppressions.parse_lines ["1+1 // @infer-ignore BUFFER_OVERRUN_L1"] |> no_error
let s2 =
Suppressions.parse_lines
["//@infer-ignore ,PULSE_UNNECESSARY_COPY"; "1+1 // @infer-ignore BUFFER_OVERRUN_L1"]
|> no_error
let s_every = Suppressions.parse_lines ["1+1 // @infer-ignore-every BUFFER_OVERRUN_L1"]
let s_every = Suppressions.parse_lines ["1+1 // @infer-ignore-every BUFFER_OVERRUN_L1"] |> no_error
let%test "matching suppression" =
Suppressions.is_suppressed ~suppressions:s1 ~issue_type:"BUFFER_OVERRUN_L1" ~line:1
Expand Down Expand Up @@ -166,7 +194,9 @@ let%test "matching suppression every large line" =
Suppressions.is_suppressed ~suppressions:s_every ~issue_type:"BUFFER_OVERRUN_L1" ~line:1000
let s_wild = Suppressions.parse_lines ["1+1 // @infer-ignore-every PULSE_UNNECESSARY_.*"]
let s_wild =
Suppressions.parse_lines ["1+1 // @infer-ignore-every PULSE_UNNECESSARY_.*"] |> no_error
let%test "matching suppression wildcard" =
Suppressions.is_suppressed ~suppressions:s_wild ~issue_type:"PULSE_UNNECESSARY_COPY_ASSIGNMENT"
Expand Down

0 comments on commit 68f7ede

Please sign in to comment.