From e73c5aea3a5ae8b57128147c621cc4f1fc6fcb44 Mon Sep 17 00:00:00 2001 From: WebFreak001 Date: Thu, 17 Mar 2022 13:42:32 +0100 Subject: [PATCH] add test for ignored & templated unittests in DMD it seems to be a syntax error to have unittests in functions or other unittests, but in libdparse it's allowed, so this dscanner check adds a warning/error for that. In the same check it implements a warning for unittests which are used in templates. However as unittests in templates could be there just for documentation, no warning is issued if annotated with a doc comment. Mixin templates are not regarded as templates in this check as they might be used for automatically generating unittests outside templates. --- src/dscanner/analysis/config.d | 3 + src/dscanner/analysis/ignored_unittest.d | 188 +++++++++++++++++++++++ src/dscanner/analysis/run.d | 5 + 3 files changed, 196 insertions(+) create mode 100644 src/dscanner/analysis/ignored_unittest.d diff --git a/src/dscanner/analysis/config.d b/src/dscanner/analysis/config.d index c3e870a2..35050fba 100644 --- a/src/dscanner/analysis/config.d +++ b/src/dscanner/analysis/config.d @@ -215,6 +215,9 @@ struct StaticAnalysisConfig @INI("Maximum cyclomatic complexity after which to issue warnings") int max_cyclomatic_complexity = 50; + @INI("Check for ignored unittests in templates or functions") + string ignored_unittest = Check.enabled; + @INI("Module-specific filters") ModuleFilters filters; } diff --git a/src/dscanner/analysis/ignored_unittest.d b/src/dscanner/analysis/ignored_unittest.d new file mode 100644 index 00000000..c9bac6bd --- /dev/null +++ b/src/dscanner/analysis/ignored_unittest.d @@ -0,0 +1,188 @@ +module dscanner.analysis.ignored_unittest; + +import dparse.lexer; +import dparse.ast; +import dscanner.analysis.base; + +import std.stdio; +import std.meta : AliasSeq; + +private string scoped(string v, string val) +{ + if (__ctfe) + { + return "auto _old_" ~ v ~ " = " ~ v ~ "; " ~ v ~ " = " ~ val ~ "; scope (exit) " ~ v ~ " = _old_" ~ v ~ ";"; + } + else + { + return null; + } +} + +/** + * unittests in methods are never run, unittests in templates may only serve for documentation + */ +final class IgnoredUnittestCheck : BaseAnalyzer +{ + enum string KEY_IGNORED = "dscanner.bugs.ignored_unittest"; + enum string MESSAGE_IGNORED = "unittest blocks can only be run in global scope or in types"; + enum string KEY_TEMPLATED = "dscanner.suspicious.templated_unittest"; + enum string MESSAGE_TEMPLATED = "unittest blocks in templates may not be run and can only serve for documentation purpose. Document with a ddoc-comment or move unittest block out of template."; + mixin AnalyzerInfo!"ignored_unittest"; + + private bool inValidScope = true; + private bool inUndocumentable; + private bool inTemplate; + + /// + this(string fileName, bool skipTests = false) + { + super(fileName, null, skipTests); + } + + static foreach (T; AliasSeq!( + ClassDeclaration, + InterfaceDeclaration, + StructDeclaration, + UnionDeclaration + )) + override void visit(const T b) + { + mixin(scoped("inValidScope", "true")); + mixin(scoped("inTemplate", "inTemplate || b.templateParameters !is null")); + b.accept(this); + } + + override void visit(const MixinTemplateDeclaration m) + { + // ignore mixin templates as they might be used for proper unittests + if (m.templateDeclaration) + { + mixin(scoped("inValidScope", "true")); + mixin(scoped("inTemplate", "false")); + m.templateDeclaration.accept(this); + } + } + + override void visit(const FunctionDeclaration b) + { + mixin(scoped("inTemplate", "inTemplate || b.templateParameters !is null")); + b.accept(this); + } + + override void visit(const FunctionBody b) + { + mixin(scoped("inValidScope", "false")); + mixin(scoped("inUndocumentable", "true")); + b.accept(this); + } + + override void visit(const TemplateDeclaration decl) + { + mixin(scoped("inTemplate", "true")); + decl.accept(this); + } + + override void visit(const Unittest test) + { + if (!inValidScope) + { + addErrorMessage(test.line, test.column, KEY_IGNORED, MESSAGE_IGNORED); + } + else if (inTemplate && inUndocumentable) + { + addErrorMessage(test.line, test.column, KEY_IGNORED, MESSAGE_IGNORED); + } + else if (inTemplate) + { + if (test.comment is null) + addErrorMessage(test.line, test.column, KEY_TEMPLATED, MESSAGE_TEMPLATED); + } + + if (!skipTests) + { + mixin(scoped("inValidScope", "false")); + mixin(scoped("inUndocumentable", "true")); + test.accept(this); + } + } + + alias visit = BaseAnalyzer.visit; +} + +unittest +{ + import std.stdio : stderr; + import std.format : format; + import dscanner.analysis.config : StaticAnalysisConfig, Check, disabledConfig; + import dscanner.analysis.helpers : assertAnalyzerWarnings; + + StaticAnalysisConfig sac = disabledConfig(); + sac.ignored_unittest = Check.enabled; + + assertAnalyzerWarnings(q{ + unittest {} + + void foo() { + unittest {} // [warn]: %s + } + + void bar()() { + unittest {} // [warn]: %s + } + + class T1() { + unittest {} // [warn]: %s + } + + class T2() { + /// + unittest {} + } + + class C { + unittest {} + } + + unittest { + unittest {} // [warn]: %s + } + + void test1() { + struct S { + unittest {} // surprisingly, this gets called + } + } + + void test2()() { + struct S { + unittest {} // [warn]: %s + } + } + + void test2() { + struct S() { + unittest {} // [warn]: %s + } + } + + mixin template MT() + { + unittest { /* ok */ } + } + + struct MixedStruct + { + mixin MT; + } + }c.format( + IgnoredUnittestCheck.MESSAGE_IGNORED, + IgnoredUnittestCheck.MESSAGE_IGNORED, + IgnoredUnittestCheck.MESSAGE_TEMPLATED, + IgnoredUnittestCheck.MESSAGE_IGNORED, + IgnoredUnittestCheck.MESSAGE_IGNORED, + IgnoredUnittestCheck.MESSAGE_IGNORED, + ), sac); + + stderr.writeln("Unittest for IgnoredUnittestCheck passed."); +} diff --git a/src/dscanner/analysis/run.d b/src/dscanner/analysis/run.d index 98459409..d26664ab 100644 --- a/src/dscanner/analysis/run.d +++ b/src/dscanner/analysis/run.d @@ -81,6 +81,7 @@ import dscanner.analysis.trust_too_much; import dscanner.analysis.redundant_storage_class; import dscanner.analysis.unused_result; import dscanner.analysis.cyclomatic_complexity; +import dscanner.analysis.ignored_unittest; import dsymbol.string_interning : internString; import dsymbol.scope_; @@ -595,6 +596,10 @@ MessageSet analyze(string fileName, const Module m, const StaticAnalysisConfig a analysisConfig.cyclomatic_complexity == Check.skipTests && !ut, analysisConfig.max_cyclomatic_complexity.to!int); + if (moduleName.shouldRun!IgnoredUnittestCheck(analysisConfig)) + checks ~= new IgnoredUnittestCheck(fileName, + analysisConfig.unused_result == Check.skipTests && !ut); + version (none) if (moduleName.shouldRun!IfStatementCheck(analysisConfig)) checks ~= new IfStatementCheck(fileName, moduleScope,