From 3bd94e41278d1c7bf29f8644114aa01d0d645225 Mon Sep 17 00:00:00 2001 From: Jeroen Engels Date: Sun, 5 Jan 2020 18:44:16 +0100 Subject: [PATCH] Replace expectErrorsForFiles by expectErrorsForModules --- src/ListExtra.elm | 22 ++++++++++-- src/Review/Test.elm | 57 ++++++++++++++++++++++---------- src/Review/Test/ErrorMessage.elm | 33 +++++++++--------- tests/ErrorMessageTest.elm | 38 ++++++++++----------- tests/NoUnusedModulesTest.elm | 21 +++++++++--- 5 files changed, 113 insertions(+), 58 deletions(-) diff --git a/src/ListExtra.elm b/src/ListExtra.elm index 4901e5f7..4f69b7b2 100644 --- a/src/ListExtra.elm +++ b/src/ListExtra.elm @@ -1,4 +1,4 @@ -module ListExtra exposing (last, uniquePairs) +module ListExtra exposing (find, last, uniquePairs) {-| Functions taken from elm-community/list-extra. @@ -8,7 +8,7 @@ elm-community/list-extra would release a new major version. # List functions -@docs last, uniquePairs +@docs find, last, uniquePairs # Original Copyright notice @@ -38,6 +38,24 @@ SOFTWARE. -} +{-| Find the first element that satisfies a predicate and return +Just that element. If none match, return Nothing. +find (\\num -> num > 5) [2, 4, 6, 8] == Just 6 +-} +find : (a -> Bool) -> List a -> Maybe a +find predicate list = + case list of + [] -> + Nothing + + first :: rest -> + if predicate first then + Just first + + else + find predicate rest + + last : List a -> Maybe a last items = case items of diff --git a/src/Review/Test.elm b/src/Review/Test.elm index 80d5b4d9..6cad243d 100644 --- a/src/Review/Test.elm +++ b/src/Review/Test.elm @@ -1,6 +1,6 @@ module Review.Test exposing ( ReviewResult, run, runWithProjectData, runMulti, runMultiWithProjectData - , ExpectedError, expectErrors, expectErrorsForFiles, expectNoErrors, error, atExactly, whenFixed + , ExpectedError, expectErrors, expectErrorsForModules, expectNoErrors, error, atExactly, whenFixed ) {-| Module that helps you test your rules, using [`elm-test`](https://package.elm-lang.org/packages/elm-explorations/test/latest/). @@ -100,7 +100,7 @@ for this module. # Making assertions -@docs ExpectedError, expectErrors, expectErrorsForFiles, expectNoErrors, error, atExactly, whenFixed +@docs ExpectedError, expectErrors, expectErrorsForModules, expectNoErrors, error, atExactly, whenFixed -} @@ -109,6 +109,7 @@ import Elm.Syntax.Module as Module import Elm.Syntax.Node as Node import Elm.Syntax.Range exposing (Range) import Expect exposing (Expectation) +import ListExtra import Review import Review.File as File import Review.Fix as Fix @@ -126,7 +127,13 @@ import Set exposing (Set) -} type ReviewResult = FailedRun String - | SuccessfulRun (List { inspector : CodeInspector, errors : List Error }) + | SuccessfulRun + (List + { moduleName : String + , inspector : CodeInspector + , errors : List Error + } + ) type alias CodeInspector = @@ -261,7 +268,12 @@ runMultiWithProjectData project rule sources = in List.map (\parsedFile -> - { inspector = codeInspectorForSource parsedFile + { moduleName = + parsedFile.ast.moduleDefinition + |> Node.value + |> Module.moduleName + |> String.join "." + , inspector = codeInspectorForSource parsedFile , errors = errors |> List.filter (\error_ -> Rule.errorFilePath error_ == parsedFile.path) @@ -438,29 +450,40 @@ an error at the end of the source code. -} expectErrors : List ExpectedError -> ReviewResult -> Expectation expectErrors expectedErrors reviewResult = - expectErrorsForFiles [ expectedErrors ] reviewResult + case reviewResult of + FailedRun errorMessage -> + Expect.fail errorMessage + + SuccessfulRun ({ inspector, errors, moduleName } :: []) -> + checkAllErrorsMatch inspector expectedErrors errors + + SuccessfulRun _ -> + Expect.fail ErrorMessage.needToUsedExpectErrorsForModules {-| TODO Documentation -} -expectErrorsForFiles : List (List ExpectedError) -> ReviewResult -> Expectation -expectErrorsForFiles expectedErrorsList reviewResult = +expectErrorsForModules : List ( String, List ExpectedError ) -> ReviewResult -> Expectation +expectErrorsForModules expectedErrorsList reviewResult = case reviewResult of FailedRun errorMessage -> Expect.fail errorMessage SuccessfulRun runResults -> - if List.length runResults /= List.length expectedErrorsList then - Expect.fail <| ErrorMessage.errorListLengthMismatch (List.length runResults) (List.length expectedErrorsList) - - else - List.map2 - (\{ inspector, errors } expectedErrors () -> - checkAllErrorsMatch inspector expectedErrors errors + runResults + |> List.map + (\{ inspector, errors, moduleName } -> + let + expectedErrors : List ExpectedError + expectedErrors = + expectedErrorsList + |> ListExtra.find (\( moduleName_, _ ) -> moduleName_ == moduleName) + |> Maybe.map Tuple.second + |> Maybe.withDefault [] + in + \() -> checkAllErrorsMatch inspector expectedErrors errors ) - runResults - expectedErrorsList - |> (\expectations -> Expect.all expectations ()) + |> (\expectations -> Expect.all expectations ()) {-| Create an expectation for an error. diff --git a/src/Review/Test/ErrorMessage.elm b/src/Review/Test/ErrorMessage.elm index d48139d8..07127659 100644 --- a/src/Review/Test/ErrorMessage.elm +++ b/src/Review/Test/ErrorMessage.elm @@ -2,7 +2,7 @@ module Review.Test.ErrorMessage exposing ( ExpectedErrorData , parsingFailure, messageMismatch, emptyDetails, unexpectedDetails, wrongLocation, didNotExpectErrors , underMismatch, expectedMoreErrors, tooManyErrors, locationIsAmbiguousInSourceCode - , errorListLengthMismatch, duplicateModuleName + , needToUsedExpectErrorsForModules, duplicateModuleName , missingFixes, unexpectedFixes, fixedCodeMismatch, unchangedSourceAfterFix, invalidSourceAfterFix, hasCollisionsInFixRanges , impossibleState ) @@ -15,7 +15,7 @@ module Review.Test.ErrorMessage exposing @docs ExpectedErrorData @docs parsingFailure, messageMismatch, emptyDetails, unexpectedDetails, wrongLocation, didNotExpectErrors @docs underMismatch, expectedMoreErrors, tooManyErrors, locationIsAmbiguousInSourceCode -@docs errorListLengthMismatch, duplicateModuleName +@docs needToUsedExpectErrorsForModules, duplicateModuleName @docs missingFixes, unexpectedFixes, fixedCodeMismatch, unchangedSourceAfterFix, invalidSourceAfterFix, hasCollisionsInFixRanges @docs impossibleState @@ -239,26 +239,27 @@ Tip: I found them at: """ ++ listOccurrencesAsLocations sourceCode under occurrencesInSourceCode -errorListLengthMismatch : Int -> Int -> String -errorListLengthMismatch expectedListLength actualListLength = - """MISMATCH BETWEEN NUMBER OF MODULES AND NUMBER OF LISTS OF ERRORS +needToUsedExpectErrorsForModules : String +needToUsedExpectErrorsForModules = + """AMBIGUOUS MODULE FOR ERROR -You passed a list of """ ++ String.fromInt expectedListLength ++ """ modules to this test, but a list of """ ++ String.fromInt actualListLength ++ """ lists -of errors. +You gave me several modules, and you expect some errors. I need to know for +which module you expect these errors to be reported. -I expect each item in the list of expected errors to correspond to the -module at the same position in the module list. Since the two lists have -different sizes, I'm not sure how to associate the last modules or errors. - -If you expect no errors to be reported for a module, use an empty list: +You should use `expectErrorsForModules` to do this: test "..." <| \\() -> - [ sourceCode1, sourceCode2 ] + [ \"\"\" +module A exposing (..) +-- someCode +\"\"\", \"\"\" +module B exposing (..) +-- someCode +\"\"\" ] |> Review.Test.runMulti rule - |> Review.Test.expectErrorsForFiles - [ [] -- Expect no errors reported in `sourceCode1` - , [ Review.Test.error theErrorForSourceCode2 ] + |> Review.Test.expectErrorsForModules + [ ( "B", [ Review.Test.error someError ] ) ]""" diff --git a/tests/ErrorMessageTest.elm b/tests/ErrorMessageTest.elm index b4678a69..c5bac36b 100644 --- a/tests/ErrorMessageTest.elm +++ b/tests/ErrorMessageTest.elm @@ -21,7 +21,7 @@ all = , expectedMoreErrorsTest , tooManyErrorsTest , locationIsAmbiguousInSourceCodeTest - , errorListLengthMismatchTest + , needToUsedExpectErrorsForModulesTest , duplicateModuleNameTest , missingFixesTest , unexpectedFixesTest @@ -607,32 +607,32 @@ Tip: I found them at: ] -errorListLengthMismatchTest : Test -errorListLengthMismatchTest = - test "errorListLengthMismatch" <| +needToUsedExpectErrorsForModulesTest : Test +needToUsedExpectErrorsForModulesTest = + test "needToUsedExpectErrorsForModules" <| \() -> - ErrorMessage.errorListLengthMismatch 1417 1418 + ErrorMessage.needToUsedExpectErrorsForModules |> expectMessageEqual """ -MISMATCH BETWEEN NUMBER OF MODULES AND NUMBER OF LISTS OF ERRORS +AMBIGUOUS MODULE FOR ERROR -You passed a list of 1417 modules to this test, but a list of 1418 lists -of errors. +You gave me several modules, and you expect some errors. I need to know for +which module you expect these errors to be reported. -I expect each item in the list of expected errors to correspond to the -module at the same position in the module list. Since the two lists have -different sizes, I'm not sure how to associate the last modules or errors. - -If you expect no errors to be reported for a module, use an empty list: +You should use `expectErrorsForModules` to do this: test "..." <| \\() -> - [ sourceCode1, sourceCode2 ] + [ \"\"\" +module A exposing (..) +-- someCode +\"\"\", \"\"\" +module B exposing (..) +-- someCode +\"\"\" ] |> Review.Test.runMulti rule - |> Review.Test.expectErrorsForFiles - [ [] -- Expect no errors reported in `sourceCode1` - , [ Review.Test.error theErrorForSourceCode2 ] - ] -""" + |> Review.Test.expectErrorsForModules + [ ( "B", [ Review.Test.error someError ] ) + ]""" duplicateModuleNameTest : Test diff --git a/tests/NoUnusedModulesTest.elm b/tests/NoUnusedModulesTest.elm index c1f051ed..7b35da75 100644 --- a/tests/NoUnusedModulesTest.elm +++ b/tests/NoUnusedModulesTest.elm @@ -89,17 +89,30 @@ main = text "" , """ module Reported exposing (..) a = 1 +""" + , """ +module Other.Reported exposing (..) +a = 1 """ ] |> Review.Test.runMultiWithProjectData application rule - |> Review.Test.expectErrorsForFiles - [ [] - , [ Review.Test.error + |> Review.Test.expectErrorsForModules + [ ( "Reported" + , [ Review.Test.error { message = "Module `Reported` is never used." , details = details , under = "Reported" } - ] + ] + ) + , ( "Other.Reported" + , [ Review.Test.error + { message = "Module `Other.Reported` is never used." + , details = details + , under = "Other.Reported" + } + ] + ) ] , test "should report a module even if it is the only module in the project" <| \() ->