From e58ce3e1bb4fc4736a38833dc40cf5727987ac5c Mon Sep 17 00:00:00 2001 From: Jon Ross-Perkins Date: Wed, 23 Oct 2024 11:16:32 -0700 Subject: [PATCH] Add coverage testing for parse node kinds. (#4436) This refactors the diagnostic kind coverage check into something that also works for node kinds. Then, since this points out a few node kinds that aren't having their parse verified, I'm adding minor tests for those. --- toolchain/diagnostics/BUILD | 8 +- toolchain/diagnostics/coverage_test.cpp | 73 ++++++++++ .../diagnostics/emitted_diagnostics_test.cpp | 130 ------------------ toolchain/parse/BUILD | 24 ++++ toolchain/parse/coverage_test.cpp | 32 +++++ .../parse/testdata/operators/infix.carbon | 43 ++++-- toolchain/testing/BUILD | 12 ++ toolchain/testing/coverage_helper.h | 82 +++++++++++ 8 files changed, 259 insertions(+), 145 deletions(-) create mode 100644 toolchain/diagnostics/coverage_test.cpp delete mode 100644 toolchain/diagnostics/emitted_diagnostics_test.cpp create mode 100644 toolchain/parse/coverage_test.cpp create mode 100644 toolchain/testing/coverage_helper.h diff --git a/toolchain/diagnostics/BUILD b/toolchain/diagnostics/BUILD index 53eb05cc61f2b..1799a0dcedb34 100644 --- a/toolchain/diagnostics/BUILD +++ b/toolchain/diagnostics/BUILD @@ -64,9 +64,9 @@ manifest( ) cc_test( - name = "emitted_diagnostics_test", + name = "coverage_test", size = "small", - srcs = ["emitted_diagnostics_test.cpp"], + srcs = ["coverage_test.cpp"], args = ["--testdata_manifest=$(location :all_testdata.txt)"], data = [ ":all_testdata.txt", @@ -74,12 +74,10 @@ cc_test( ], deps = [ ":diagnostic_kind", - "//common:set", "//testing/base:gtest_main", + "//toolchain/testing:coverage_helper", "@abseil-cpp//absl/flags:flag", "@googletest//:gtest", - "@llvm-project//llvm:Support", - "@re2", ], ) diff --git a/toolchain/diagnostics/coverage_test.cpp b/toolchain/diagnostics/coverage_test.cpp new file mode 100644 index 0000000000000..26076d8b6cb37 --- /dev/null +++ b/toolchain/diagnostics/coverage_test.cpp @@ -0,0 +1,73 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#include + +#include "absl/flags/flag.h" +#include "toolchain/diagnostics/diagnostic_kind.h" +#include "toolchain/testing/coverage_helper.h" + +ABSL_FLAG(std::string, testdata_manifest, "", + "A path to a file containing repo-relative names of test files."); + +namespace Carbon { +namespace { + +constexpr DiagnosticKind DiagnosticKinds[] = { +#define CARBON_DIAGNOSTIC_KIND(Name) DiagnosticKind::Name, +#include "toolchain/diagnostics/diagnostic_kind.def" +}; + +constexpr DiagnosticKind UntestedDiagnosticKinds[] = { + // These exist only for unit tests. + DiagnosticKind::TestDiagnostic, + DiagnosticKind::TestDiagnosticNote, + + // These diagnose filesystem issues that are hard to unit test. + DiagnosticKind::ErrorReadingFile, + DiagnosticKind::ErrorStattingFile, + DiagnosticKind::FileTooLarge, + + // Int literals are currently limited to i32. Once that's fixed, this + // should be tested. + DiagnosticKind::ArrayBoundTooLarge, + + // This isn't feasible to test with a normal testcase, but is tested in + // lex/tokenized_buffer_test.cpp. + DiagnosticKind::TooManyTokens, + + // TODO: Should look closer at these, but adding tests is a high risk of + // loss in merge conflicts due to the amount of tests being changed right + // now. + DiagnosticKind::ExternLibraryInImporter, + DiagnosticKind::ExternLibraryOnDefinition, + DiagnosticKind::HexadecimalEscapeMissingDigits, + DiagnosticKind::ImplOfUndefinedInterface, + DiagnosticKind::IncompleteTypeInFunctionParam, + DiagnosticKind::InvalidDigit, + DiagnosticKind::InvalidDigitSeparator, + DiagnosticKind::InvalidHorizontalWhitespaceInString, + DiagnosticKind::MismatchedIndentInString, + DiagnosticKind::ModifierPrivateNotAllowed, + DiagnosticKind::MultiLineStringWithDoubleQuotes, + DiagnosticKind::NameAmbiguousDueToExtend, + DiagnosticKind::TooManyDigits, + DiagnosticKind::UnaryOperatorRequiresWhitespace, + DiagnosticKind::UnicodeEscapeSurrogate, + DiagnosticKind::UnicodeEscapeTooLarge, + DiagnosticKind::UnknownBaseSpecifier, + DiagnosticKind::UnsupportedCRLineEnding, + DiagnosticKind::UnsupportedLFCRLineEnding, +}; + +// Looks for diagnostic kinds that aren't covered by a file_test. +TEST(Coverage, DiagnosticKind) { + Testing::TestKindCoverage(absl::GetFlag(FLAGS_testdata_manifest), + R"(^ *// CHECK:STDERR: .*\.carbon:.* \[(\w+)\]$)", + llvm::ArrayRef(DiagnosticKinds), + llvm::ArrayRef(UntestedDiagnosticKinds)); +} + +} // namespace +} // namespace Carbon diff --git a/toolchain/diagnostics/emitted_diagnostics_test.cpp b/toolchain/diagnostics/emitted_diagnostics_test.cpp deleted file mode 100644 index f015689064ffa..0000000000000 --- a/toolchain/diagnostics/emitted_diagnostics_test.cpp +++ /dev/null @@ -1,130 +0,0 @@ -// Part of the Carbon Language project, under the Apache License v2.0 with LLVM -// Exceptions. See /LICENSE for license information. -// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception - -#include - -#include -#include - -#include "absl/flags/flag.h" -#include "common/set.h" -#include "llvm/ADT/StringExtras.h" -#include "re2/re2.h" -#include "toolchain/diagnostics/diagnostic_kind.h" - -ABSL_FLAG(std::string, testdata_manifest, "", - "A path to a file containing repo-relative names of test files."); - -namespace Carbon { -namespace { - -constexpr DiagnosticKind Diagnostics[] = { -#define CARBON_DIAGNOSTIC_KIND(Name) DiagnosticKind::Name, -#include "toolchain/diagnostics/diagnostic_kind.def" -}; - -// Returns true for diagnostics which have no tests. In general, diagnostics -// should be tested. -static auto IsUntestedDiagnostic(DiagnosticKind diagnostic_kind) -> bool { - switch (diagnostic_kind) { - case DiagnosticKind::TestDiagnostic: - case DiagnosticKind::TestDiagnosticNote: - // These exist only for unit tests. - return true; - case DiagnosticKind::ErrorReadingFile: - case DiagnosticKind::ErrorStattingFile: - case DiagnosticKind::FileTooLarge: - // These diagnose filesystem issues that are hard to unit test. - return true; - case DiagnosticKind::ArrayBoundTooLarge: - // Int literals are currently limited to i32. Once that's fixed, this - // should be tested. - return true; - case DiagnosticKind::ExternLibraryInImporter: - case DiagnosticKind::ExternLibraryOnDefinition: - case DiagnosticKind::HexadecimalEscapeMissingDigits: - case DiagnosticKind::ImplOfUndefinedInterface: - case DiagnosticKind::IncompleteTypeInFunctionParam: - case DiagnosticKind::InvalidDigit: - case DiagnosticKind::InvalidDigitSeparator: - case DiagnosticKind::InvalidHorizontalWhitespaceInString: - case DiagnosticKind::MismatchedIndentInString: - case DiagnosticKind::ModifierPrivateNotAllowed: - case DiagnosticKind::MultiLineStringWithDoubleQuotes: - case DiagnosticKind::NameAmbiguousDueToExtend: - case DiagnosticKind::TooManyDigits: - case DiagnosticKind::UnaryOperatorRequiresWhitespace: - case DiagnosticKind::UnicodeEscapeSurrogate: - case DiagnosticKind::UnicodeEscapeTooLarge: - case DiagnosticKind::UnknownBaseSpecifier: - case DiagnosticKind::UnsupportedCRLineEnding: - case DiagnosticKind::UnsupportedLFCRLineEnding: - // TODO: Should look closer at these, but adding tests is a high risk of - // loss in merge conflicts due to the amount of tests being changed right - // now. - return true; - case DiagnosticKind::TooManyTokens: - // This isn't feasible to test with a normal testcase, but is tested in - // lex/tokenized_buffer_test.cpp. - return true; - default: - return false; - } -} - -TEST(EmittedDiagnostics, Verify) { - std::ifstream manifest_in(absl::GetFlag(FLAGS_testdata_manifest)); - ASSERT_TRUE(manifest_in.good()); - - RE2 diagnostic_re(R"(^ *// CHECK:STDERR: .*\.carbon:.* \[(\w+)\]$)"); - ASSERT_TRUE(diagnostic_re.ok()) << diagnostic_re.error(); - - Set emitted_diagnostics; - - std::string test_filename; - while (std::getline(manifest_in, test_filename)) { - std::ifstream test_in(test_filename); - ASSERT_TRUE(test_in.good()); - - std::string line; - while (std::getline(test_in, line)) { - std::string diagnostic; - if (RE2::PartialMatch(line, diagnostic_re, &diagnostic)) { - emitted_diagnostics.Insert(diagnostic); - } - } - } - - llvm::SmallVector missing_diagnostics; - for (auto diagnostic_kind : Diagnostics) { - if (IsUntestedDiagnostic(diagnostic_kind)) { - EXPECT_FALSE(emitted_diagnostics.Erase(diagnostic_kind.name())) - << diagnostic_kind - << " was previously untested, and is now tested. That's good, but " - "please remove it from IsUntestedDiagnostic."; - continue; - } - if (!emitted_diagnostics.Erase(diagnostic_kind.name())) { - missing_diagnostics.push_back(diagnostic_kind.name()); - } - } - - constexpr llvm::StringLiteral Bullet = "\n - "; - - std::sort(missing_diagnostics.begin(), missing_diagnostics.end()); - EXPECT_TRUE(missing_diagnostics.empty()) - << "Some diagnostics have no tests:" << Bullet - << llvm::join(missing_diagnostics, Bullet); - - llvm::SmallVector unexpected_matches; - emitted_diagnostics.ForEach( - [&](const std::string& match) { unexpected_matches.push_back(match); }); - std::sort(unexpected_matches.begin(), unexpected_matches.end()); - EXPECT_TRUE(unexpected_matches.empty()) - << "Matched things that don't appear to be diagnostics:" << Bullet - << llvm::join(unexpected_matches, Bullet); -} - -} // namespace -} // namespace Carbon diff --git a/toolchain/parse/BUILD b/toolchain/parse/BUILD index 5f7ba0327fee1..8e72b4174a286 100644 --- a/toolchain/parse/BUILD +++ b/toolchain/parse/BUILD @@ -3,6 +3,7 @@ # SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception load("@rules_cc//cc:defs.bzl", "cc_library", "cc_test") +load("//bazel/manifest:defs.bzl", "manifest") load("//testing/fuzzing:rules.bzl", "cc_fuzz_test") package(default_visibility = ["//visibility:public"]) @@ -12,6 +13,11 @@ filegroup( data = glob(["testdata/**/*.carbon"]), ) +manifest( + name = "testdata.txt", + srcs = [":testdata"], +) + cc_library( name = "node_kind", srcs = ["node_kind.cpp"], @@ -198,3 +204,21 @@ cc_test( "@googletest//:gtest", ], ) + +cc_test( + name = "coverage_test", + size = "small", + srcs = ["coverage_test.cpp"], + args = ["--testdata_manifest=$(location :testdata.txt)"], + data = [ + ":testdata", + ":testdata.txt", + ], + deps = [ + ":node_kind", + "//testing/base:gtest_main", + "//toolchain/testing:coverage_helper", + "@abseil-cpp//absl/flags:flag", + "@googletest//:gtest", + ], +) diff --git a/toolchain/parse/coverage_test.cpp b/toolchain/parse/coverage_test.cpp new file mode 100644 index 0000000000000..9a1c49f9dc93b --- /dev/null +++ b/toolchain/parse/coverage_test.cpp @@ -0,0 +1,32 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#include + +#include "absl/flags/flag.h" +#include "toolchain/parse/node_kind.h" +#include "toolchain/testing/coverage_helper.h" + +ABSL_FLAG(std::string, testdata_manifest, "", + "A path to a file containing repo-relative names of test files."); + +namespace Carbon::Parse { +namespace { + +constexpr NodeKind NodeKinds[] = { +#define CARBON_PARSE_NODE_KIND(Name) NodeKind::Name, +#include "toolchain/parse/node_kind.def" +}; + +constexpr NodeKind UntestedNodeKinds[] = {NodeKind::Placeholder}; + +// Looks for node kinds that aren't covered by a file_test. +TEST(Coverage, NodeKind) { + Testing::TestKindCoverage(absl::GetFlag(FLAGS_testdata_manifest), + R"(kind: '(\w+)')", llvm::ArrayRef(NodeKinds), + llvm::ArrayRef(UntestedNodeKinds)); +} + +} // namespace +} // namespace Carbon::Parse diff --git a/toolchain/parse/testdata/operators/infix.carbon b/toolchain/parse/testdata/operators/infix.carbon index ca7013a0540e5..b4913514daf3c 100644 --- a/toolchain/parse/testdata/operators/infix.carbon +++ b/toolchain/parse/testdata/operators/infix.carbon @@ -8,19 +8,42 @@ // TIP: To dump output, run: // TIP: bazel run //toolchain/testing:file_test -- --dump_output --file_tests=toolchain/parse/testdata/operators/infix.carbon -var n: i8 = n * n; +fn F() { + n * n; + n ^ n; + n >= n; + n >> n; + n / n; +} // CHECK:STDOUT: - filename: infix.carbon // CHECK:STDOUT: parse_tree: [ // CHECK:STDOUT: {kind: 'FileStart', text: ''}, -// CHECK:STDOUT: {kind: 'VariableIntroducer', text: 'var'}, -// CHECK:STDOUT: {kind: 'IdentifierName', text: 'n'}, -// CHECK:STDOUT: {kind: 'IntTypeLiteral', text: 'i8'}, -// CHECK:STDOUT: {kind: 'BindingPattern', text: ':', subtree_size: 3}, -// CHECK:STDOUT: {kind: 'VariableInitializer', text: '='}, -// CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'n'}, -// CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'n'}, -// CHECK:STDOUT: {kind: 'InfixOperatorStar', text: '*', subtree_size: 3}, -// CHECK:STDOUT: {kind: 'VariableDecl', text: ';', subtree_size: 9}, +// CHECK:STDOUT: {kind: 'FunctionIntroducer', text: 'fn'}, +// CHECK:STDOUT: {kind: 'IdentifierName', text: 'F'}, +// CHECK:STDOUT: {kind: 'TuplePatternStart', text: '('}, +// CHECK:STDOUT: {kind: 'TuplePattern', text: ')', subtree_size: 2}, +// CHECK:STDOUT: {kind: 'FunctionDefinitionStart', text: '{', subtree_size: 5}, +// CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'n'}, +// CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'n'}, +// CHECK:STDOUT: {kind: 'InfixOperatorStar', text: '*', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'ExprStatement', text: ';', subtree_size: 4}, +// CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'n'}, +// CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'n'}, +// CHECK:STDOUT: {kind: 'InfixOperatorCaret', text: '^', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'ExprStatement', text: ';', subtree_size: 4}, +// CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'n'}, +// CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'n'}, +// CHECK:STDOUT: {kind: 'InfixOperatorGreaterEqual', text: '>=', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'ExprStatement', text: ';', subtree_size: 4}, +// CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'n'}, +// CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'n'}, +// CHECK:STDOUT: {kind: 'InfixOperatorGreaterGreater', text: '>>', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'ExprStatement', text: ';', subtree_size: 4}, +// CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'n'}, +// CHECK:STDOUT: {kind: 'IdentifierNameExpr', text: 'n'}, +// CHECK:STDOUT: {kind: 'InfixOperatorSlash', text: '/', subtree_size: 3}, +// CHECK:STDOUT: {kind: 'ExprStatement', text: ';', subtree_size: 4}, +// CHECK:STDOUT: {kind: 'FunctionDefinition', text: '}', subtree_size: 26}, // CHECK:STDOUT: {kind: 'FileEnd', text: ''}, // CHECK:STDOUT: ] diff --git a/toolchain/testing/BUILD b/toolchain/testing/BUILD index 42f35f2f22c51..11e2edc3126d9 100644 --- a/toolchain/testing/BUILD +++ b/toolchain/testing/BUILD @@ -37,6 +37,18 @@ cc_library( ], ) +cc_library( + name = "coverage_helper", + testonly = 1, + hdrs = ["coverage_helper.h"], + deps = [ + "//common:set", + "@googletest//:gtest", + "@llvm-project//llvm:Support", + "@re2", + ], +) + file_test( name = "file_test", size = "small", diff --git a/toolchain/testing/coverage_helper.h b/toolchain/testing/coverage_helper.h new file mode 100644 index 0000000000000..e91dc5792c999 --- /dev/null +++ b/toolchain/testing/coverage_helper.h @@ -0,0 +1,82 @@ +// Part of the Carbon Language project, under the Apache License v2.0 with LLVM +// Exceptions. See /LICENSE for license information. +// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception + +#ifndef CARBON_TOOLCHAIN_TESTING_COVERAGE_HELPER_H_ +#define CARBON_TOOLCHAIN_TESTING_COVERAGE_HELPER_H_ + +#include + +#include +#include + +#include "common/set.h" +#include "llvm/ADT/StringExtras.h" +#include "re2/re2.h" + +namespace Carbon::Testing { + +// Looks for kinds that aren't covered by a file_test in the manifest path. +// Kinds are identified by the provided regular expression kind_pattern. +// +// should_be_covered should return false when a kind is either untestable or not +// yet tested. +template +auto TestKindCoverage(const std::string& manifest_path, + llvm::StringLiteral kind_pattern, + llvm::ArrayRef kinds, + llvm::ArrayRef untested_kinds) { + std::ifstream manifest_in(manifest_path.c_str()); + ASSERT_TRUE(manifest_in.good()); + + RE2 kind_re(kind_pattern.data()); + ASSERT_TRUE(kind_re.ok()) << kind_re.error(); + + Set covered_kinds; + + std::string test_filename; + while (std::getline(manifest_in, test_filename)) { + std::ifstream test_in(test_filename); + ASSERT_TRUE(test_in.good()); + + std::string line; + while (std::getline(test_in, line)) { + std::string kind; + if (RE2::PartialMatch(line, kind_re, &kind)) { + covered_kinds.Insert(kind); + } + } + } + + llvm::SmallVector missing_kinds; + for (auto kind : kinds) { + if (llvm::find(untested_kinds, kind) != untested_kinds.end()) { + EXPECT_FALSE(covered_kinds.Erase(kind.name())) + << "Kind " << kind + << " has coverage even though none was expected. If this has " + "changed, update the coverage test."; + continue; + } + if (!covered_kinds.Erase(kind.name())) { + missing_kinds.push_back(kind.name()); + } + } + + constexpr llvm::StringLiteral Bullet = "\n - "; + + std::sort(missing_kinds.begin(), missing_kinds.end()); + EXPECT_TRUE(missing_kinds.empty()) << "Some kinds have no tests:" << Bullet + << llvm::join(missing_kinds, Bullet); + + llvm::SmallVector unexpected_matches; + covered_kinds.ForEach( + [&](const std::string& match) { unexpected_matches.push_back(match); }); + std::sort(unexpected_matches.begin(), unexpected_matches.end()); + EXPECT_TRUE(unexpected_matches.empty()) + << "Matched things that aren't in the kind list:" << Bullet + << llvm::join(unexpected_matches, Bullet); +} + +} // namespace Carbon::Testing + +#endif // CARBON_TOOLCHAIN_TESTING_COVERAGE_HELPER_H_