From 11b37494a919c00c226a65b0fe5aa064db6eeea2 Mon Sep 17 00:00:00 2001 From: Jeroen Engels Date: Sun, 18 Jun 2017 20:37:57 +0200 Subject: [PATCH] Add rule ElmTest.NoDuplicateTestBodies --- LintConfig.elm | 2 + README.md | 1 + elm-package.json | 3 +- example/Main.elm | 2 + .../Rules/ElmTest/NoDuplicateTestBodies.elm | 183 +++++++++++++++ tests/ElmTest/NoDuplicateTestBodiesTest.elm | 213 ++++++++++++++++++ 6 files changed, 403 insertions(+), 1 deletion(-) create mode 100644 src/Lint/Rules/ElmTest/NoDuplicateTestBodies.elm create mode 100644 tests/ElmTest/NoDuplicateTestBodiesTest.elm diff --git a/LintConfig.elm b/LintConfig.elm index d3a696b6..5e53a3d7 100644 --- a/LintConfig.elm +++ b/LintConfig.elm @@ -15,6 +15,7 @@ import Lint.Rules.NoUselessPatternMatching import Lint.Rules.NoWarningComments import Lint.Rules.SimplifyPiping import Lint.Rules.SimplifyPropertyAccess +import Lint.Rules.ElmTest.NoDuplicateTestBodies config : List ( Severity, LintRule ) @@ -33,4 +34,5 @@ config = , ( Warning, Lint.Rules.NoWarningComments.rule ) , ( Critical, Lint.Rules.SimplifyPiping.rule ) , ( Critical, Lint.Rules.SimplifyPropertyAccess.rule ) + , ( Critical, Lint.Rules.ElmTest.NoDuplicateTestBodies.rule ) ] diff --git a/README.md b/README.md index e5bcbef0..c40ca430 100644 --- a/README.md +++ b/README.md @@ -33,6 +33,7 @@ You can read the slides for my [presentation](http://slides.com/jeroenengels/elm - [NoWarningComments](rules/NoWarningComments.md) - Detect comments containing words like `TODO`, `FIXME` and `XXX`. - [SimplifyPiping](rules/SimplifyPiping.md) - Simplify piped functions like `List.map f >> List.map g` to `List.map (f >> g)` - [SimplifyPropertyAccess](rules/SimplifyPropertyAccess.md) - Replace functions that only return the property of its parameter by an access function, like `(\x -> x.foo)` to `.foo` +- [ElmTest.NoDuplicateTestBodies](rules/ElmTest/NoDuplicateTestBodies.md) - Forbid having multiple tests with the same bodies. Often a consequence of copy-pasting tests. More rule ideas in this [slide](http://slides.com/jeroenengels/elm-lint#/5/3) and the ones below it. Note that some rules were implemented but may not be good ideas. Think for yourself and ask the community whether you should enable them. diff --git a/elm-package.json b/elm-package.json index 0ff755d6..b4964347 100644 --- a/elm-package.json +++ b/elm-package.json @@ -22,7 +22,8 @@ "Lint.Rules.NoUselessPatternMatching", "Lint.Rules.NoWarningComments", "Lint.Rules.SimplifyPiping", - "Lint.Rules.SimplifyPropertyAccess" + "Lint.Rules.SimplifyPropertyAccess", + "Lint.Rules.ElmTest.NoDuplicateTestBodies" ], "dependencies": { "Bogdanp/elm-ast": "8.0.3 <= v < 9.0.0", diff --git a/example/Main.elm b/example/Main.elm index a5b584be..c9fc2ac3 100644 --- a/example/Main.elm +++ b/example/Main.elm @@ -30,6 +30,7 @@ import Lint.Rules.NoUselessPatternMatching import Lint.Rules.NoWarningComments import Lint.Rules.SimplifyPiping import Lint.Rules.SimplifyPropertyAccess +import Lint.Rules.ElmTest.NoDuplicateTestBodies type Msg @@ -52,6 +53,7 @@ config = , ( Warning, Lint.Rules.NoWarningComments.rule ) , ( Critical, Lint.Rules.SimplifyPiping.rule ) , ( Critical, Lint.Rules.SimplifyPropertyAccess.rule ) + , ( Critical, Lint.Rules.ElmTest.NoDuplicateTestBodies.rule ) ] diff --git a/src/Lint/Rules/ElmTest/NoDuplicateTestBodies.elm b/src/Lint/Rules/ElmTest/NoDuplicateTestBodies.elm new file mode 100644 index 00000000..a67349c6 --- /dev/null +++ b/src/Lint/Rules/ElmTest/NoDuplicateTestBodies.elm @@ -0,0 +1,183 @@ +module Lint.Rules.ElmTest.NoDuplicateTestBodies exposing (rule) + +{-| +@docs rule + +# Fail + + module Addition exposing (..) + + import Test exposing (test) + + tests = + [ test "foo" <| + \() -> 1 + 1 + |> Expect.equal 2 + , test "bar" <| + \() -> 1 + 1 + |> Expect.equal 2 + ] + +# Success + + module Addition exposing (..) + + import Test exposing (test) + + tests = + [ test "foo" <| + \() -> 1 + 1 + |> Expect.equal 2 + , test "bar" <| + \() -> 1 + 2 + |> Expect.equal 3 + ] +-} + +import Ast.Expression exposing (..) +import Ast.Statement exposing (..) +import Lint exposing (lint, doNothing) +import Lint.Types exposing (LintRule, LintRuleImplementation, LintError, Direction(..)) +import Dict exposing (Dict) + + +type alias Context = + { availableTestAliases : List String + } + + +{-| Forbid dupicate test bodies. +When copy-pasting tests, it can happen that the title is changed but the developer forgets to update the test body. +This may result in specifications that are thought to be implemented but are not enforced by tests. + + rules = + [ ElmTest.NoDuplicateTestBodies.rule + ] +-} +rule : LintRule +rule input = + lint input implementation + + +implementation : LintRuleImplementation Context +implementation = + { statementFn = statementFn + , typeFn = doNothing + , expressionFn = expressionFn + , moduleEndFn = (\ctx -> ( [], ctx )) + , initialContext = Context [] + } + + +error : ( String, String ) -> LintError +error ( title1, title2 ) = + LintError + "ElmTest.NoDuplicateTestBodies" + ("Test `" ++ title1 ++ "` has the same body as test `" ++ title2 ++ "`") + + +isTestFunctionCall : List String -> Expression -> Bool +isTestFunctionCall availableTestAliases expr = + case expr of + Variable fnName -> + List.member (String.join "." fnName) availableTestAliases + + Access (Variable object) fields -> + List.member (String.join "." <| object ++ fields) availableTestAliases + + _ -> + False + + +filterTests : List String -> List Expression -> List ( String, Expression ) +filterTests availableTestAliases listItems = + List.concatMap + (\item -> + case item of + BinOp (Variable [ "<|" ]) (Application fn (String title)) testBody -> + if isTestFunctionCall availableTestAliases fn then + [ ( title, testBody ) ] + else + [] + + _ -> + [] + ) + listItems + + +expressionFn : Context -> Direction Expression -> ( List LintError, Context ) +expressionFn ctx node = + case node of + Enter (List listItems) -> + let + tests = + filterTests ctx.availableTestAliases listItems + + redundantTests = + List.foldl + (\( title, testBody ) { dict, redundant } -> + let + testBodyAsString = + toString testBody + + existingTest = + Dict.get testBodyAsString dict + in + case existingTest of + Nothing -> + { dict = Dict.insert testBodyAsString title dict, redundant = redundant } + + Just existingTestTitle -> + { dict = dict, redundant = redundant ++ [ ( title, existingTestTitle ) ] } + ) + { dict = Dict.empty, redundant = [] } + tests + in + ( List.map error redundantTests.redundant, ctx ) + + _ -> + ( [], ctx ) + + +extractImported : ExportSet -> List String +extractImported exportSet = + case exportSet of + AllExport -> + [ "test" ] + + SubsetExport list -> + List.concatMap extractImported list + + FunctionExport name -> + if name == "test" then + [ name ] + else + [] + + _ -> + [] + + +computeAlias : Maybe String -> String +computeAlias = + Maybe.withDefault "Test" + + +statementFn : Context -> Direction Statement -> ( List LintError, Context ) +statementFn ctx node = + case node of + Enter (ImportStatement [ "Test" ] testAlias exportSet) -> + let + moduleFnAccess = + computeAlias testAlias ++ ".test" + in + case exportSet of + Nothing -> + ( [], { availableTestAliases = [ moduleFnAccess ] } ) + + Just subExportSet -> + ( [], { availableTestAliases = [ moduleFnAccess ] ++ extractImported subExportSet } ) + + _ -> + ( [], ctx ) diff --git a/tests/ElmTest/NoDuplicateTestBodiesTest.elm b/tests/ElmTest/NoDuplicateTestBodiesTest.elm new file mode 100644 index 00000000..6dc3d6cb --- /dev/null +++ b/tests/ElmTest/NoDuplicateTestBodiesTest.elm @@ -0,0 +1,213 @@ +module ElmTest.NoDuplicateTestBodiesTest exposing (all) + +import Test exposing (describe, test, Test) +import Lint.Rules.ElmTest.NoDuplicateTestBodies exposing (rule) +import Lint.Types exposing (LintRule, LintError, LintResult) +import TestUtil exposing (ruleTester, expectErrors) + + +testRule : String -> LintResult +testRule = + ruleTester rule + + +error : String -> LintError +error = + LintError "ElmTest.NoDuplicateTestBodies" + + +tests : List Test +tests = + [ test "should not report non-tests" <| + \() -> + testRule """ + foo = [ 1, 1 ] + bar = [ foo 2 <| 1, foo 2 <| 1 ] + tests = + [ fn "foo" <| + \\() -> 1 + 1 + |> Expect.equal 2 + , fn "bar" <| + \\() -> 1 + 2 + |> Expect.equal 3 + ] + """ + |> expectErrors [] + , test "should not report tests that have distinct bodies" <| + \() -> + testRule """ + tests = + [ test "foo" <| + \\() -> 1 + 1 + |> Expect.equal 2 + , test "bar" <| + \\() -> 1 + 2 + |> Expect.equal 3 + ] + """ + |> expectErrors [] + , test "should not report tests that have the same body when Test is not imported and used in list" <| + \() -> + testRule """ + tests = + [ test "foo" <| + \\() -> 1 + 1 + |> Expect.equal 2 + , test "bar" <| + \\() -> 1 + 1 + |> Expect.equal 2 + ] + """ + |> expectErrors [] + , test "should report tests that have the same body when using test and exposing (test)" <| + \() -> + testRule """ + import Test exposing (test, foo) + tests = + [ test "foo" <| + \\() -> 1 + 1 + |> Expect.equal 2 + , test "bar" <| + \\() -> 1 + 1 + |> Expect.equal 2 + , test "baz" <| + \\() -> 1 + 1 + |> Expect.equal 2 + ] + """ + |> expectErrors + [ error "Test `bar` has the same body as test `foo`" + , error "Test `baz` has the same body as test `foo`" + ] + , test "should report tests that have the same body when using Test.test and exposing (test)" <| + \() -> + testRule """ + import Test exposing (test, foo) + tests = + [ Test.test "foo" <| + \\() -> 1 + 1 + |> Expect.equal 2 + , Test.test "bar" <| + \\() -> 1 + 1 + |> Expect.equal 2 + ] + """ + |> expectErrors [ error "Test `bar` has the same body as test `foo`" ] + , test "should report tests that have the same body when using test and exposing (..)" <| + \() -> + testRule """ + import Test exposing (..) + tests = + [ test "foo" <| + \\() -> 1 + 1 + |> Expect.equal 2 + , test "bar" <| + \\() -> 1 + 1 + |> Expect.equal 2 + ] + """ + |> expectErrors [ error "Test `bar` has the same body as test `foo`" ] + , test "should report tests that have the same body when using Test.test and exposing (..)" <| + \() -> + testRule """ + import Test exposing (..) + tests = + [ Test.test "foo" <| + \\() -> 1 + 1 + |> Expect.equal 2 + , Test.test "bar" <| + \\() -> 1 + 1 + |> Expect.equal 2 + ] + """ + |> expectErrors [ error "Test `bar` has the same body as test `foo`" ] + , test "should report tests that have the same body when using Foo.test, aliasing as Foo and exposing (..)" <| + \() -> + testRule """ + import Test as Foo exposing (..) + tests = + [ Foo.test "foo" <| + \\() -> 1 + 1 + |> Expect.equal 2 + , Foo.test "bar" <| + \\() -> 1 + 1 + |> Expect.equal 2 + ] + """ + |> expectErrors [ error "Test `bar` has the same body as test `foo`" ] + , test "should report tests that have the same body when using Test.test without exposing anything" <| + \() -> + testRule """ + import Test + tests = + [ Test.test "foo" <| + \\() -> 1 + 1 + |> Expect.equal 2 + , Test.test "bar" <| + \\() -> 1 + 1 + |> Expect.equal 2 + ] + """ + |> expectErrors [ error "Test `bar` has the same body as test `foo`" ] + , test "should report tests that have the same body when using Foo.test aliasing as Foo and without exposing anything" <| + \() -> + testRule """ + import Test as Foo + tests = + [ Foo.test "foo" <| + \\() -> 1 + 1 + |> Expect.equal 2 + , Foo.test "bar" <| + \\() -> 1 + 1 + |> Expect.equal 2 + ] + """ + |> expectErrors [ error "Test `bar` has the same body as test `foo`" ] + , test "should not report tests that have the same body when using test and not exposing (test)" <| + \() -> + testRule """ + import Test exposing (foo) + tests = + [ test "foo" <| + \\() -> 1 + 1 + |> Expect.equal 2 + , test "bar" <| + \\() -> 1 + 1 + |> Expect.equal 2 + ] + """ + |> expectErrors [] + , test "should not report tests that have the same body when using test without exposing anything" <| + \() -> + testRule """ + import Test + tests = + [ test "foo" <| + \\() -> 1 + 1 + |> Expect.equal 2 + , test "bar" <| + \\() -> 1 + 1 + |> Expect.equal 2 + ] + """ + |> expectErrors [] + , test "should not report tests that have the same body when using test and aliasing as Foo without exposing anything" <| + \() -> + testRule """ + import Test as Foo + tests = + [ Test.test "foo" <| + \\() -> 1 + 1 + |> Expect.equal 2 + , Test.test "bar" <| + \\() -> 1 + 1 + |> Expect.equal 2 + ] + """ + |> expectErrors [] + ] + + +all : Test +all = + describe "ElmTest.NoDuplicateTestBodies" tests