From 919467fee636cabd098492154de9b69a1c6715ae Mon Sep 17 00:00:00 2001 From: Jeroen Engels Date: Mon, 5 Aug 2019 00:04:27 +0200 Subject: [PATCH] Add a way to check in tests that the fixed code is correct --- src/Lint/Rule.elm | 4 ++ src/Lint/Test.elm | 44 ++++++++++-- src/Lint/Test/ErrorMessage.elm | 58 +++++++++++++++- tests/ErrorMessageTest.elm | 117 ++++++++++++++++++++++++++++++++ tests/NoUnusedVariablesTest.elm | 55 ++++++++++++++- 5 files changed, 271 insertions(+), 7 deletions(-) diff --git a/src/Lint/Rule.elm b/src/Lint/Rule.elm index 35843982..b337066a 100644 --- a/src/Lint/Rule.elm +++ b/src/Lint/Rule.elm @@ -1081,6 +1081,10 @@ error { message, details } range = Take a look at [`Lint.Fix`](./Lint-Fix) to know more on how to makes fixes. +If you pass `withFixes` an empty list, the error will be considered as having no +automatic fix available. ALso, calling `withFixes` several times on an error will +overwrite the previous fixes. + **Note**: Each fix applies on a location in the code, defined by a range. To avoid an unpredictable result, those ranges may not overlap. The order of the fixes does not matter. diff --git a/src/Lint/Test.elm b/src/Lint/Test.elm index a2b6eed2..19a09a43 100644 --- a/src/Lint/Test.elm +++ b/src/Lint/Test.elm @@ -1,6 +1,6 @@ module Lint.Test exposing ( LintResult, run - , ExpectedError, expectErrors, expectNoErrors, error, atExactly + , ExpectedError, expectErrors, expectNoErrors, error, atExactly, whenFixed ) {-| Module that helps you test your linting rules, using [`elm-test`](https://package.elm-lang.org/packages/elm-explorations/test/latest). @@ -78,7 +78,7 @@ changes drastically. # Making assertions -@docs ExpectedError, expectErrors, expectNoErrors, error, atExactly +@docs ExpectedError, expectErrors, expectNoErrors, error, atExactly, whenFixed -} @@ -86,6 +86,7 @@ import Array exposing (Array) import Elm.Syntax.Range exposing (Range) import Expect exposing (Expectation) import Lint exposing (lintSource) +import Lint.Fix as Fix import Lint.Rule as Rule exposing (Error, Rule) import Lint.Test.ErrorMessage as ErrorMessage @@ -98,7 +99,8 @@ type LintResult type alias CodeInspector = - { getCodeAtLocation : Range -> Maybe String + { source : String + , getCodeAtLocation : Range -> Maybe String , checkIfLocationIsAmbiguous : Error -> String -> Expectation } @@ -110,6 +112,7 @@ type ExpectedError { message : String , details : List String , under : Under + , fixedSource : Maybe String } @@ -154,9 +157,11 @@ run rule source = , details = Lint.errorDetails error_ } (Lint.errorRange error_) + |> Rule.withFixes (Lint.fixes error_ |> Maybe.withDefault []) ) |> SuccessfulRun - { getCodeAtLocation = getCodeAtLocationInSourceCode source + { source = source + , getCodeAtLocation = getCodeAtLocationInSourceCode source , checkIfLocationIsAmbiguous = checkIfLocationIsAmbiguousInSourceCode source } @@ -274,6 +279,7 @@ error input = { message = input.message , details = input.details , under = Under input.under + , fixedSource = Nothing } @@ -315,6 +321,11 @@ atExactly range ((ExpectedError expectedError_) as expectedError) = ExpectedError { expectedError_ | under = UnderExactly (getUnder expectedError) range } +whenFixed : String -> ExpectedError -> ExpectedError +whenFixed fixedSource ((ExpectedError expectedError_) as expectedError) = + ExpectedError { expectedError_ | fixedSource = Just fixedSource } + + getUnder : ExpectedError -> String getUnder (ExpectedError expectedError) = case expectedError.under of @@ -413,6 +424,7 @@ checkErrorMatch codeInspector ((ExpectedError expectedError_) as expectedError) |> always , checkMessageAppearsUnder codeInspector error_ expectedError , checkDetailsAreCorrect error_ expectedError + , always <| checkFixesAreCorrect codeInspector error_ expectedError ] @@ -457,6 +469,30 @@ checkDetailsAreCorrect error_ (ExpectedError expectedError) = ] +checkFixesAreCorrect : CodeInspector -> Error -> ExpectedError -> Expectation +checkFixesAreCorrect codeInspector error_ ((ExpectedError expectedError_) as expectedError) = + case ( expectedError_.fixedSource, Rule.errorFixes error_ ) of + ( Nothing, Nothing ) -> + Expect.pass + + ( Just expectedFixedSource, Nothing ) -> + ErrorMessage.missingFixes (extractExpectedErrorData expectedError) + |> Expect.fail + + ( Nothing, Just fixes ) -> + ErrorMessage.unexpectedFixes error_ + |> Expect.fail + + ( Just expectedFixedSource, Just fixes ) -> + case Fix.fix fixes codeInspector.source of + Fix.Successful fixedSource -> + (fixedSource == expectedFixedSource) + |> Expect.true (ErrorMessage.fixedCodeMismatch fixedSource expectedFixedSource error_) + + Fix.Errored _ -> + Expect.fail "Supplied fixes caused a problem" + + extractExpectedErrorData : ExpectedError -> ErrorMessage.ExpectedErrorData extractExpectedErrorData ((ExpectedError expectedErrorContent) as expectedError) = { message = expectedErrorContent.message diff --git a/src/Lint/Test/ErrorMessage.elm b/src/Lint/Test/ErrorMessage.elm index 30efb47c..460c9709 100644 --- a/src/Lint/Test/ErrorMessage.elm +++ b/src/Lint/Test/ErrorMessage.elm @@ -2,6 +2,7 @@ module Lint.Test.ErrorMessage exposing ( ExpectedErrorData , parsingFailure, messageMismatch, emptyDetails, unexpectedDetails, wrongLocation, didNotExpectErrors , underMismatch, expectedMoreErrors, tooManyErrors, locationIsAmbiguousInSourceCode + , missingFixes, unexpectedFixes, fixedCodeMismatch , impossibleState ) @@ -13,6 +14,7 @@ module Lint.Test.ErrorMessage exposing @docs ExpectedErrorData @docs parsingFailure, messageMismatch, emptyDetails, unexpectedDetails, wrongLocation, didNotExpectErrors @docs underMismatch, expectedMoreErrors, tooManyErrors, locationIsAmbiguousInSourceCode +@docs missingFixes, unexpectedFixes, fixedCodeMismatch @docs impossibleState -} @@ -220,6 +222,53 @@ impossibleState = Oh no! I'm in an impossible state. I found an error at a location that I could not find back. Please let me know and give me an SSCCE (http://sscce.org/) here: https://github.com/jfmengels/elm-lint/issues.""" +missingFixes : ExpectedErrorData -> String +missingFixes expectedError = + """MISSING FIXES + +I expected that the error with the following message + + """ ++ wrapInQuotes expectedError.message ++ """ + +would provide some fixes, but I didn't find any. + +Hint: It's probable that you either forgot to call `Rule.withFixes` on the +error that you created, or that the list of provided fixes was empty.""" + + +unexpectedFixes : Error -> String +unexpectedFixes error = + """UNEXPECTED FIXES + +I expected that the error with the following message + + """ ++ wrapInQuotes (Rule.errorMessage error) ++ """ + +would not have any fixes, but it provided some. + +Hint: You may have forgotten to call `Lint.Test.whenFixed` +It's probable that you either forgot to call `Rule.withFixes` on the +error that you created, or that the list of provided fixes was empty.""" + + +fixedCodeMismatch : SourceCode -> SourceCode -> Error -> String +fixedCodeMismatch resultingSourceCode expectedSourceCode error = + """FIXED CODE MISMATCH + +I found a different fixed source code than expected for the error with the +following message: + + """ ++ wrapInQuotes (Rule.errorMessage error) ++ """ + +I found the following result after the fixes have been applied: + + """ ++ formatSourceCode resultingSourceCode ++ """ + +but I was expecting: + + """ ++ formatSourceCode expectedSourceCode + + -- STYLIZING AND FORMATTING @@ -235,7 +284,14 @@ formatSourceCode string = else lines - |> List.map (\str -> " " ++ str) + |> List.map + (\str -> + if str == "" then + "" + + else + " " ++ str + ) |> String.join "\n" |> (\str -> "```\n" ++ str ++ "\n ```") diff --git a/tests/ErrorMessageTest.elm b/tests/ErrorMessageTest.elm index 742bcbff..e922ad9b 100644 --- a/tests/ErrorMessageTest.elm +++ b/tests/ErrorMessageTest.elm @@ -2,6 +2,7 @@ module ErrorMessageTest exposing (all) import Elm.Syntax.Range exposing (Range) import Expect exposing (Expectation) +import Lint.Fix as Fix import Lint.Rule as Rule exposing (Error) import Lint.Test.ErrorMessage as ErrorMessage exposing (ExpectedErrorData) import Test exposing (Test, describe, test) @@ -20,6 +21,9 @@ all = , expectedMoreErrorsTest , tooManyErrorsTest , locationIsAmbiguousInSourceCodeTest + , missingFixesTest + , unexpectedFixesTest + , fixedCodeMismatchTest ] @@ -580,6 +584,119 @@ Tip: I found them at: ] +missingFixesTest : Test +missingFixesTest = + test "missingFixes" <| + \() -> + let + expectedError : ExpectedErrorData + expectedError = + { message = "Some error" + , details = [ "Some details" ] + , under = "Debug.log" + } + in + ErrorMessage.missingFixes expectedError + |> expectMessageEqual """ +MISSING FIXES + +I expected that the error with the following message + + `Some error` + +would provide some fixes, but I didn't find any. + +Hint: It's probable that you either forgot to call `Rule.withFixes` on the +error that you created, or that the list of provided fixes was empty.""" + + +unexpectedFixesTest : Test +unexpectedFixesTest = + test "unexpectedFixes" <| + \() -> + let + range : Range + range = + { start = { row = 3, column = 1 }, end = { row = 4, column = 3 } } + + error : Error + error = + Rule.error + { message = "Some error" + , details = [ "Some details" ] + } + range + |> Rule.withFixes [ Fix.removeRange range ] + in + ErrorMessage.unexpectedFixes error + |> expectMessageEqual """ +UNEXPECTED FIXES + +I expected that the error with the following message + + `Some error` + +would not have any fixes, but it provided some. + +Hint: You may have forgotten to call `Lint.Test.whenFixed` +It's probable that you either forgot to call `Rule.withFixes` on the +error that you created, or that the list of provided fixes was empty.""" + + +fixedCodeMismatchTest : Test +fixedCodeMismatchTest = + test "fixedCodeMismatch" <| + \() -> + let + sourceCode : String + sourceCode = + """module A exposing (b) +abcd = + 1""" + + expectedSourceCode : String + expectedSourceCode = + """module A exposing (b) +abcd = + 2""" + + error : Error + error = + Rule.error + { message = "Some error" + , details = [ "Some details" ] + } + { start = { row = 3, column = 1 }, end = { row = 3, column = 5 } } + in + ErrorMessage.fixedCodeMismatch + sourceCode + expectedSourceCode + error + |> expectMessageEqual """ +FIXED CODE MISMATCH + +I found a different fixed source code than expected for the error with the +following message: + + `Some error` + +I found the following result after the fixes have been applied: + + ``` + module A exposing (b) + abcd = + 1 + ``` + +but I was expecting: + + ``` + module A exposing (b) + abcd = + 2 + ```""" + + dummyRange : Range dummyRange = { start = { row = 2, column = 1 }, end = { row = 2, column = 5 } } diff --git a/tests/NoUnusedVariablesTest.elm b/tests/NoUnusedVariablesTest.elm index 0b40aa6a..9b829d4b 100644 --- a/tests/NoUnusedVariablesTest.elm +++ b/tests/NoUnusedVariablesTest.elm @@ -48,19 +48,41 @@ b = a 1""" , test "should report unused top-level variables" <| \() -> testRule """module SomeModule exposing (b) -a = 1""" +b = 1 +a = 2""" |> Lint.Test.expectErrors [ Lint.Test.error { message = "Variable `a` is not used" , details = details , under = "a" } + |> Lint.Test.whenFixed """module SomeModule exposing (b) +b = 1 +""" + ] + , test "should report unused top-level variables with type annotation" <| + \() -> + testRule """module SomeModule exposing (b) +b = 1 +a : Int +a = 2""" + |> Lint.Test.expectErrors + [ Lint.Test.error + { message = "Variable `a` is not used" + , details = details + , under = "a" + } + |> Lint.Test.atExactly { start = { row = 4, column = 1 }, end = { row = 4, column = 2 } } + |> Lint.Test.whenFixed """module SomeModule exposing (b) +b = 1 +""" ] , test "should report unused top-level variables even if they are annotated" <| \() -> testRule """module SomeModule exposing (b) a: Int -a = 1""" +a = 1 +b = 2""" |> Lint.Test.expectErrors [ Lint.Test.error { message = "Variable `a` is not used" @@ -68,6 +90,9 @@ a = 1""" , under = "a" } |> Lint.Test.atExactly { start = { row = 3, column = 1 }, end = { row = 3, column = 2 } } + |> Lint.Test.whenFixed """module SomeModule exposing (b) + +b = 2""" ] , test "should not report unused top-level variables if everything is exposed" <| \() -> @@ -93,6 +118,10 @@ c = 3""" , details = details , under = "c" } + |> Lint.Test.whenFixed """module SomeModule exposing (a, b) +a = 1 +b = 2 +""" ] , test "should not report unused top-level variables if everything is exposed (port module)" <| \() -> @@ -118,6 +147,10 @@ c = 3""" , details = details , under = "c" } + |> Lint.Test.whenFixed """port module SomeModule exposing (a, b) +a = 1 +b = 2 +""" ] , test "should report unused variable even if a homonym from a module is used" <| \() -> @@ -131,6 +164,9 @@ a = Html.Styled.Attributes.href""" , under = "href" } |> Lint.Test.atExactly { start = { row = 2, column = 1 }, end = { row = 2, column = 5 } } + |> Lint.Test.whenFixed """module SomeModule exposing (a) + +a = Html.Styled.Attributes.href""" ] ] @@ -148,6 +184,7 @@ a = let b = 1 , details = details , under = "b" } + |> Lint.Test.whenFixed "module SomeModule exposing (a)\na = let \n in 2" ] , test "should report unused variables from let even if they are exposed by name" <| \() -> @@ -161,6 +198,7 @@ a = let b = 1 , under = "b" } |> Lint.Test.atExactly { start = { row = 2, column = 9 }, end = { row = 2, column = 10 } } + |> Lint.Test.whenFixed "module SomeModule exposing (a, b)\na = let \n in 2" ] , test "should report unused functions from let even if they are exposed by name" <| \() -> @@ -174,6 +212,7 @@ a = let b param = 1 , under = "b" } |> Lint.Test.atExactly { start = { row = 2, column = 9 }, end = { row = 2, column = 10 } } + |> Lint.Test.whenFixed "module SomeModule exposing (a, b)\na = let \n in 2" ] , test "should report unused variables from let even if everything is exposed" <| \() -> @@ -186,6 +225,7 @@ a = let b = 1 , details = details , under = "b" } + |> Lint.Test.whenFixed "module SomeModule exposing (..)\na = let \n in 2" ] , test "should not report variables from let declarations that are used in the expression" <| \() -> @@ -257,6 +297,10 @@ a = { b | c = 3 }""" , under = "c" } |> Lint.Test.atExactly { start = { row = 3, column = 1 }, end = { row = 3, column = 2 } } + |> Lint.Test.whenFixed """module SomeModule exposing (a) +b = { z = 1, c = 2 } + +a = { b | c = 3 }""" ] ] @@ -675,6 +719,9 @@ type A a = B a""" , under = "a" } |> Lint.Test.atExactly { start = { row = 2, column = 1 }, end = { row = 2, column = 2 } } + |> Lint.Test.whenFixed """module SomeModule exposing (A) + +type A a = B a""" ] , test "should report unused variable even if it is present in a generic record type" <| \() -> @@ -689,6 +736,10 @@ a str = {c = str}""" , under = "r" } |> Lint.Test.atExactly { start = { row = 2, column = 1 }, end = { row = 2, column = 2 } } + |> Lint.Test.whenFixed """module SomeModule exposing (a) + +a : { r | c: A } +a str = {c = str}""" ] ]