From c76d4d972f092295b6450dc8f36f2f00ccf3ef28 Mon Sep 17 00:00:00 2001 From: Jeroen Engels Date: Sun, 17 May 2020 11:47:52 +0200 Subject: [PATCH] Make expected errors order-agnostic Fixes #12 --- src/Review/Test.elm | 134 +++++++- tests/NoUnused/CustomTypeConstructorsTest.elm | 10 +- tests/Review/Test/ExpectedErrorOrderTest.elm | 296 ++++++++++++++++++ 3 files changed, 426 insertions(+), 14 deletions(-) create mode 100644 tests/Review/Test/ExpectedErrorOrderTest.elm diff --git a/src/Review/Test.elm b/src/Review/Test.elm index 5ebfb2a2..f5f0bbcc 100644 --- a/src/Review/Test.elm +++ b/src/Review/Test.elm @@ -145,12 +145,15 @@ type alias CodeInspector = {-| An expectation for an error. Use [`error`](#error) to create one. -} type ExpectedError - = ExpectedError - { message : String - , details : List String - , under : Under - , fixedSource : Maybe String - } + = ExpectedError ExpectedErrorDetails + + +type alias ExpectedErrorDetails = + { message : String + , details : List String + , under : Under + , fixedSource : Maybe String + } type Under @@ -946,9 +949,120 @@ checkIfLocationIsAmbiguousInSourceCode sourceCode error_ under = -- RUNNING THE CHECKS +type alias ReorderState = + { expectedErrors : List ExpectedError + , reviewErrors : List ReviewError + , pairs : List ( ExpectedError, ReviewError ) + , expectedErrorsWithNoMatch : List ExpectedError + } + + +reorderErrors : CodeInspector -> ReorderState -> ( List ExpectedError, List ReviewError ) +reorderErrors codeInspector reorderState = + case reorderState.expectedErrors of + [] -> + ( List.reverse <| reorderState.expectedErrorsWithNoMatch ++ List.map Tuple.first reorderState.pairs + , List.reverse <| reorderState.reviewErrors ++ List.map Tuple.second reorderState.pairs + ) + + ((ExpectedError expectedErrorDetails) as expectedError) :: restOfExpectedErrors -> + case findBestMatchingReviewError codeInspector expectedErrorDetails reorderState.reviewErrors { error = Nothing, confidenceLevel = 0 } of + Just reviewError -> + reorderErrors codeInspector + { reorderState + | pairs = ( expectedError, reviewError ) :: reorderState.pairs + , reviewErrors = removeFirstOccurrence reviewError reorderState.reviewErrors + , expectedErrors = restOfExpectedErrors + } + + Nothing -> + reorderErrors codeInspector + { reorderState + | expectedErrorsWithNoMatch = expectedError :: reorderState.expectedErrorsWithNoMatch + , expectedErrors = restOfExpectedErrors + } + + +removeFirstOccurrence : a -> List a -> List a +removeFirstOccurrence elementToRemove list = + case list of + [] -> + [] + + x :: xs -> + if x == elementToRemove then + xs + + else + x :: removeFirstOccurrence elementToRemove xs + + +findBestMatchingReviewError : CodeInspector -> ExpectedErrorDetails -> List ReviewError -> { error : Maybe ReviewError, confidenceLevel : Int } -> Maybe ReviewError +findBestMatchingReviewError codeInspector expectedErrorDetails reviewErrors bestMatch = + case reviewErrors of + [] -> + bestMatch.error + + reviewError :: restOfReviewErrors -> + let + confidenceLevel : Int + confidenceLevel = + matchingConfidenceLevel codeInspector expectedErrorDetails reviewError + in + if confidenceLevel > bestMatch.confidenceLevel then + findBestMatchingReviewError + codeInspector + expectedErrorDetails + restOfReviewErrors + { error = Just reviewError, confidenceLevel = confidenceLevel } + + else + findBestMatchingReviewError + codeInspector + expectedErrorDetails + restOfReviewErrors + bestMatch + + +matchingConfidenceLevel : CodeInspector -> ExpectedErrorDetails -> ReviewError -> Int +matchingConfidenceLevel codeInspector expectedErrorDetails reviewError = + if expectedErrorDetails.message /= Rule.errorMessage reviewError then + 0 + + else + case expectedErrorDetails.under of + Under under -> + if codeInspector.getCodeAtLocation (Rule.errorRange reviewError) /= Just under then + 1 + + else + 2 + + UnderExactly under range -> + if codeInspector.getCodeAtLocation (Rule.errorRange reviewError) /= Just under then + 1 + + else if range /= Rule.errorRange reviewError then + 2 + + else + 3 + + checkAllErrorsMatch : SuccessfulRunResult -> List ExpectedError -> Expectation -checkAllErrorsMatch runResult expectedErrors = - checkErrorsMatch runResult expectedErrors runResult.errors +checkAllErrorsMatch runResult unorderedExpectedErrors = + let + ( expectedErrors, reviewErrors ) = + reorderErrors + runResult.inspector + { expectedErrors = unorderedExpectedErrors + , reviewErrors = runResult.errors + , pairs = [] + , expectedErrorsWithNoMatch = [] + } + in + --checkErrorsMatch runResult unorderedExpectedErrors runResult.errors + checkErrorsMatch runResult expectedErrors reviewErrors |> List.reverse |> (\expectations -> Expect.all expectations ()) @@ -972,6 +1086,7 @@ checkErrorsMatch runResult expectedErrors errors = checkErrorMatch : CodeInspector -> ExpectedError -> ReviewError -> (() -> Expectation) checkErrorMatch codeInspector ((ExpectedError expectedError_) as expectedError) error_ = + -- TODO Look here for comparison Expect.all [ \() -> (expectedError_.message == Rule.errorMessage error_) @@ -1007,7 +1122,8 @@ checkMessageAppearsUnder codeInspector error_ (ExpectedError expectedError) = , \() -> (codeAtLocation == under) |> Expect.true (FailureMessage.underMismatch error_ { under = under, codeAtLocation = codeAtLocation }) - , \() -> codeInspector.checkIfLocationIsAmbiguous error_ under + , \() -> + codeInspector.checkIfLocationIsAmbiguous error_ under ] UnderExactly under range -> diff --git a/tests/NoUnused/CustomTypeConstructorsTest.elm b/tests/NoUnused/CustomTypeConstructorsTest.elm index bea063ea..083b804e 100644 --- a/tests/NoUnused/CustomTypeConstructorsTest.elm +++ b/tests/NoUnused/CustomTypeConstructorsTest.elm @@ -142,15 +142,15 @@ type Foo = Bar | Baz""" |> Review.Test.runWithProjectData project (rule []) |> Review.Test.expectErrors [ Review.Test.error - { message = "Type constructor `Bar` is not used." - , details = details - , under = "Bar" - } - , Review.Test.error { message = "Type constructor `Baz` is not used." , details = details , under = "Baz" } + , Review.Test.error + { message = "Type constructor `Bar` is not used." + , details = details + , under = "Bar" + } ] ] diff --git a/tests/Review/Test/ExpectedErrorOrderTest.elm b/tests/Review/Test/ExpectedErrorOrderTest.elm new file mode 100644 index 00000000..21c2d065 --- /dev/null +++ b/tests/Review/Test/ExpectedErrorOrderTest.elm @@ -0,0 +1,296 @@ +module Review.Test.ExpectedErrorOrderTest exposing (all) + +import Elm.Syntax.Expression as Expression exposing (Expression) +import Elm.Syntax.Node as Node exposing (Node) +import Review.Rule as Rule exposing (Error, Rule) +import Review.Test +import Test exposing (Test, describe, test) + + +all : Test +all = + describe "Review.Test - order of tests" + [ differentMessageTest + , sameMessageTest + ] + + +differentMessageTest : Test +differentMessageTest = + describe "when message is different for every error" + [ test "should match a single error to the single expected error" <| + \() -> + """module MyModule exposing (b) +a = foo""" + |> Review.Test.run ruleWithDifferentMessage + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Detected foo" + , details = details + , under = "foo" + } + ] + , test "should match multiple errors with the expected errors that are already in the correct order" <| + \() -> + """module MyModule exposing (b) +a = foo + bar + baz +""" + |> Review.Test.run ruleWithDifferentMessage + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Detected foo" + , details = details + , under = "foo" + } + , Review.Test.error + { message = "Detected bar" + , details = details + , under = "bar" + } + , Review.Test.error + { message = "Detected baz" + , details = details + , under = "baz" + } + ] + , test "should match multiple errors with the expected errors that are in reverse order" <| + \() -> + """module MyModule exposing (b) +a = foo + bar + baz +""" + |> Review.Test.run ruleWithDifferentMessage + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Detected baz" + , details = details + , under = "baz" + } + , Review.Test.error + { message = "Detected bar" + , details = details + , under = "bar" + } + , Review.Test.error + { message = "Detected foo" + , details = details + , under = "foo" + } + ] + , test "should match multiple errors with the expected errors that are in a 'random' order" <| + \() -> + """module MyModule exposing (b) +a = foo + bar + baz +""" + |> Review.Test.run ruleWithDifferentMessage + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Detected bar" + , details = details + , under = "bar" + } + , Review.Test.error + { message = "Detected baz" + , details = details + , under = "baz" + } + , Review.Test.error + { message = "Detected foo" + , details = details + , under = "foo" + } + ] + , test "should match multiple errors with the same details and 'under' if they are in the correct order" <| + \() -> + """module MyModule exposing (b) +a = foo + foo + foo +""" + |> Review.Test.run ruleWithDifferentMessage + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Detected foo" + , details = details + , under = "foo" + } + |> Review.Test.atExactly { start = { row = 2, column = 5 }, end = { row = 2, column = 8 } } + , Review.Test.error + { message = "Detected foo" + , details = details + , under = "foo" + } + |> Review.Test.atExactly { start = { row = 3, column = 5 }, end = { row = 3, column = 8 } } + , Review.Test.error + { message = "Detected foo" + , details = details + , under = "foo" + } + |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 8 } } + ] + , test "should match multiple errors with the same details and 'under' if they are in a random order" <| + \() -> + """module MyModule exposing (b) +a = foo + foo + foo +""" + |> Review.Test.run ruleWithDifferentMessage + |> Review.Test.expectErrors + [ Review.Test.error + { message = "Detected foo" + , details = details + , under = "foo" + } + |> Review.Test.atExactly { start = { row = 3, column = 5 }, end = { row = 3, column = 8 } } + , Review.Test.error + { message = "Detected foo" + , details = details + , under = "foo" + } + |> Review.Test.atExactly { start = { row = 4, column = 5 }, end = { row = 4, column = 8 } } + , Review.Test.error + { message = "Detected foo" + , details = details + , under = "foo" + } + |> Review.Test.atExactly { start = { row = 2, column = 5 }, end = { row = 2, column = 8 } } + ] + ] + + +sameMessageTest : Test +sameMessageTest = + describe "when message is the same for every error" + [ test "should match a single error to the single expected error" <| + \() -> + """module MyModule exposing (b) +a = foo""" + |> Review.Test.run ruleWithSameMessage + |> Review.Test.expectErrors + [ Review.Test.error + { message = "message" + , details = details + , under = "foo" + } + ] + , test "should match multiple errors with the expected errors that are already in the correct order" <| + \() -> + """module MyModule exposing (b) +a = foo + bar + baz +""" + |> Review.Test.run ruleWithSameMessage + |> Review.Test.expectErrors + [ Review.Test.error + { message = "message" + , details = details + , under = "foo" + } + , Review.Test.error + { message = "message" + , details = details + , under = "bar" + } + , Review.Test.error + { message = "message" + , details = details + , under = "baz" + } + ] + , test "should match multiple errors with the expected errors that are in reverse order" <| + \() -> + """module MyModule exposing (b) +a = foo + bar + baz +""" + |> Review.Test.run ruleWithSameMessage + |> Review.Test.expectErrors + [ Review.Test.error + { message = "message" + , details = details + , under = "baz" + } + , Review.Test.error + { message = "message" + , details = details + , under = "bar" + } + , Review.Test.error + { message = "message" + , details = details + , under = "foo" + } + ] + , test "should match multiple errors with the expected errors that are in a 'random' order" <| + \() -> + """module MyModule exposing (b) +a = foo + bar + baz +""" + |> Review.Test.run ruleWithSameMessage + |> Review.Test.expectErrors + [ Review.Test.error + { message = "message" + , details = details + , under = "bar" + } + , Review.Test.error + { message = "message" + , details = details + , under = "baz" + } + , Review.Test.error + { message = "message" + , details = details + , under = "foo" + } + ] + ] + + + +-- TODO with different locations for the same element (with atexactly) +-- TEST RULES + + +ruleWithDifferentMessage : Rule +ruleWithDifferentMessage = + Rule.newModuleRuleSchema "NoValues" () + |> Rule.withSimpleExpressionVisitor (expressionVisitor (\fnName -> "Detected " ++ fnName)) + |> Rule.fromModuleRuleSchema + + +ruleWithSameMessage : Rule +ruleWithSameMessage = + Rule.newModuleRuleSchema "NoValues" () + |> Rule.withSimpleExpressionVisitor (expressionVisitor (always "message")) + |> Rule.fromModuleRuleSchema + + +expressionVisitor : (String -> String) -> Node Expression -> List (Error {}) +expressionVisitor messageFn node = + case Node.value node of + Expression.FunctionOrValue _ fnName -> + [ Rule.error + { message = messageFn fnName + , details = details + } + (Node.range node) + ] + + _ -> + [] + + +details : List String +details = + [ "details" ]