Skip to content

Commit

Permalink
Rewrite generated file checker tests
Browse files Browse the repository at this point in the history
Summary:
This splits up the generated file checker tests into 3, uses `.arcunit`
to specify includes when they should be triggered, uses a base class for the
shared code and cleans up some of the bazel querying to figure out targets.

Test Plan:
Made changes that should trigger the unit tests. Ran `arc unit`

Before running generate scripts:
{F132932}

After running generate scripts:
{F132933}

Reviewers: michelle, zasgar

Reviewed By: michelle

Differential Revision: https://phab.corp.pixielabs.ai/D8138

GitOrigin-RevId: 9658700
  • Loading branch information
vihangm committed Apr 13, 2021
1 parent 2849465 commit d283511
Show file tree
Hide file tree
Showing 13 changed files with 216 additions and 257 deletions.
3 changes: 1 addition & 2 deletions .arcconfig
Expand Up @@ -4,6 +4,5 @@
"arc.land.onto": ["main"],
"base": "git:merge-base(origin/main), arc:prompt",
"arc.feature.start.default": "origin/main",
"load": ["./arc_addons"],
"unit.engine": "PLTestEngine"
"load": ["./arc_addons"]
}
5 changes: 1 addition & 4 deletions .arclint
Expand Up @@ -181,10 +181,7 @@
"type": "xhpast",
"include": [
"(^arc_addons/.*\\.php$)"
],
"severity": {
"5": "warning"
}
]
},
"yaml": {
"type": "script-and-regex",
Expand Down
34 changes: 34 additions & 0 deletions .arcunit
@@ -0,0 +1,34 @@
{
"engines": {
"proto-gen-checker": {
"type": "proto-gen-checker",
"include": [
"(\\.proto$)"
],
"exclude": [
"(^experimental/)",
"(^third_party/)"
]
},
"graphql-gen-checker": {
"type": "graphql-gen-checker",
"include": [
"(\\.graphql$)"
],
"exclude": [
"(^experimental/)",
"(^third_party/)"
]
},
"go-gen-checker": {
"type": "go-gen-checker",
"include": [
"(\\.go$)"
],
"exclude": [
"(^experimental/)",
"(^third_party/)"
]
}
}
}
12 changes: 8 additions & 4 deletions arc_addons/__phutil_library_map__.php
Expand Up @@ -10,29 +10,33 @@
'__library_version__' => 2,
'class' => array(
'ArcaistTypescriptLinter' => 'ts/lint/ArcaistTypescriptLinter.php',
'ArcanistBaseGenCheckerTestEngine' => 'pixielabs/unit/ArcanistBaseGenCheckerTestEngine.php',
'ArcanistClangFormatLinter' => 'clang_format/lint/ArcanistClangFormatLinter.php',
'ArcanistESLintLinter' => 'js/lint/ArcanistESLintLinter.php',
'ArcanistGoGenCheckerTestEngine' => 'pixielabs/unit/ArcanistGoGenCheckerTestEngine.php',
'ArcanistGoImportsLinter' => 'pixielabs/lint/ArcanistGoImportsLinter.php',
'ArcanistGoVetLinter' => 'golang/lint/ArcanistGoVetLinter.php',
'ArcanistGolangCiLinter' => 'pixielabs/lint/ArcanistGolangCiLinter.php',
'ArcanistGraphqlGenCheckerTestEngine' => 'pixielabs/unit/ArcanistGraphqlGenCheckerTestEngine.php',
'ArcanistProtoBreakCheckLinter' => 'pixielabs/lint/ArcanistProtoBreakCheckLinter.php',
'ArcanistProtoGenCheckerTestEngine' => 'pixielabs/unit/ArcanistProtoGenCheckerTestEngine.php',
'ArcanistShellCheckLinter' => 'shellcheck/lint/ArcanistShellCheckLinter.php',
'ArcanistShellCheckLinterTestCase' => 'shellcheck/lint/__tests__/ArcanistShellCheckLinterTestCase.php',
'FileCheckerTestEngine' => 'pixielabs/unit/FileCheckerTestEngine.php',
'GoGenerateCheckerTestEngine' => 'pixielabs/unit/GoGenerateCheckerTestEngine.php',
'PLTestEngine' => 'pixielabs/unit/PLTestEngine.php',
),
'function' => array(),
'xmap' => array(
'ArcaistTypescriptLinter' => 'ArcanistExternalLinter',
'ArcanistBaseGenCheckerTestEngine' => 'ArcanistUnitTestEngine',
'ArcanistClangFormatLinter' => 'ArcanistExternalLinter',
'ArcanistESLintLinter' => 'ArcanistExternalLinter',
'ArcanistGoGenCheckerTestEngine' => 'ArcanistBaseGenCheckerTestEngine',
'ArcanistGoImportsLinter' => 'ArcanistExternalLinter',
'ArcanistGoVetLinter' => 'ArcanistExternalLinter',
'ArcanistGolangCiLinter' => 'ArcanistExternalLinter',
'ArcanistGraphqlGenCheckerTestEngine' => 'ArcanistBaseGenCheckerTestEngine',
'ArcanistProtoBreakCheckLinter' => 'ArcanistExternalLinter',
'ArcanistProtoGenCheckerTestEngine' => 'ArcanistBaseGenCheckerTestEngine',
'ArcanistShellCheckLinter' => 'ArcanistExternalLinter',
'ArcanistShellCheckLinterTestCase' => 'PhutilTestCase',
'PLTestEngine' => 'ArcanistUnitTestEngine',
),
));
12 changes: 8 additions & 4 deletions arc_addons/pixielabs/__phutil_library_map__.php
Expand Up @@ -9,18 +9,22 @@
phutil_register_library_map(array(
'__library_version__' => 2,
'class' => array(
'ArcanistBaseGenCheckerTestEngine' => 'unit/ArcanistBaseGenCheckerTestEngine.php',
'ArcanistGoGenCheckerTestEngine' => 'unit/ArcanistGoGenCheckerTestEngine.php',
'ArcanistGoImportsLinter' => 'lint/ArcanistGoImportsLinter.php',
'ArcanistGolangCiLinter' => 'lint/ArcanistGolangCiLinter.php',
'ArcanistGraphqlGenCheckerTestEngine' => 'unit/ArcanistGraphqlGenCheckerTestEngine.php',
'ArcanistProtoBreakCheckLinter' => 'lint/ArcanistProtoBreakCheckLinter.php',
'FileCheckerTestEngine' => 'unit/FileCheckerTestEngine.php',
'GoGenerateCheckerTestEngine' => 'unit/GoGenerateCheckerTestEngine.php',
'PLTestEngine' => 'unit/PLTestEngine.php',
'ArcanistProtoGenCheckerTestEngine' => 'unit/ArcanistProtoGenCheckerTestEngine.php',
),
'function' => array(),
'xmap' => array(
'ArcanistBaseGenCheckerTestEngine' => 'ArcanistUnitTestEngine',
'ArcanistGoGenCheckerTestEngine' => 'ArcanistBaseGenCheckerTestEngine',
'ArcanistGoImportsLinter' => 'ArcanistExternalLinter',
'ArcanistGolangCiLinter' => 'ArcanistExternalLinter',
'ArcanistGraphqlGenCheckerTestEngine' => 'ArcanistBaseGenCheckerTestEngine',
'ArcanistProtoBreakCheckLinter' => 'ArcanistExternalLinter',
'PLTestEngine' => 'ArcanistUnitTestEngine',
'ArcanistProtoGenCheckerTestEngine' => 'ArcanistBaseGenCheckerTestEngine',
),
));
31 changes: 31 additions & 0 deletions arc_addons/pixielabs/unit/ArcanistBaseGenCheckerTestEngine.php
@@ -0,0 +1,31 @@
<?php

abstract class ArcanistBaseGenCheckerTestEngine extends ArcanistUnitTestEngine {
public function checkFile($source_file, $file_to_check, $instructions) {
$source_path = $this->getWorkingCopy()->getProjectRoot().DIRECTORY_SEPARATOR.$source_file;
$check_path = $this->getWorkingCopy()->getProjectRoot().DIRECTORY_SEPARATOR.$file_to_check;

$res = new ArcanistUnitTestResult();
$res->setName(get_class($this).':'.$source_file);
if (!file_exists($check_path)) {
// Generating files is optional. Lack of a generated file means there isn't anything to check in.
$res->setResult(ArcanistUnitTestResult::RESULT_SKIP);
return $res;
}

if (filemtime($check_path) >= filemtime($source_path)) {
$res->setUserData($file_to_check.' is newer than '.$source_file.' file from which it was generated');
$res->setResult(ArcanistUnitTestResult::RESULT_PASS);
} else {
$res->setUserData($instructions."\n".$file_to_check.' is older than '.$source_file.' file from which it was generated');
$res->setResult(ArcanistUnitTestResult::RESULT_FAIL);
}
return $res;
}

public function shouldEchoTestResults() {
// This func/flag is badly named. Setting it to false, says that we don't render results
// and that ArcanistConfigurationDrivenUnitTestEngine should do so instead.
return false;
}
}
56 changes: 56 additions & 0 deletions arc_addons/pixielabs/unit/ArcanistGoGenCheckerTestEngine.php
@@ -0,0 +1,56 @@
<?php

final class ArcanistGoGenCheckerTestEngine extends ArcanistBaseGenCheckerTestEngine {
private $goGenerateMap = array(
'go-bindata' => '/(?<=-o=)(.*)(?=\.gen\.go)/',
'mockgen' => '/(?<=-destination=)(.*)(?=\.gen\.go)/',
'genny' => '/(?<=-out )(.*)(?=\.gen\.go)/',
);

public function getEngineConfigurationName() {
return 'go-gen-checker';
}

public function shouldEchoTestResults() {
// This func/flag is badly named. Setting it to false, says that we don't render results
// and that ArcanistConfigurationDrivenUnitTestEngine should do so instead.
return false;
}

public function run() {
$test_results = array();

foreach ($this->getPaths() as $file) {
$file_path = $this->getWorkingCopy()->getProjectRoot().DIRECTORY_SEPARATOR.$file;

// Find if the .go file contains //go:generate.
foreach (file($file_path) as $line_num => $line) {
if (strpos($line, '//go:generate') !== false) {
$command = preg_split('/\s+/', $line)[1];

if (!array_key_exists($command, $this->goGenerateMap)) {
$res = new ArcanistUnitTestResult();
$res->setName(get_class($this));
$res->setResult(ArcanistUnitTestResult::RESULT_FAIL);
$res->setUserData('go:generate command '.$command.' has not been added to goGenerateMap. Please add'.
' an entry in $goGenerateMap in linters/engine/FileCheckerTestEngine.php, '.
'where the key is '.$command.' and the value is a regex for the name of the'.
' generated output file.');
$test_results[] = $res;
break;
}

// Find the name of the .gen.go output file.
$matches = array();
preg_match($this->goGenerateMap[$command], $line, $matches);
$gen_go_filename = substr($file, 0, strrpos($file, '/') + 1).$matches[0].'.gen.go';

$test_results[] = $this->checkFile($file, $gen_go_filename, 'To regenerate, run "go generate" in the appropriate directory.');
break;
}
}
}

return $test_results;
}
}
18 changes: 18 additions & 0 deletions arc_addons/pixielabs/unit/ArcanistGraphqlGenCheckerTestEngine.php
@@ -0,0 +1,18 @@
<?php

final class ArcanistGraphqlGenCheckerTestEngine extends ArcanistBaseGenCheckerTestEngine {
public function getEngineConfigurationName() {
return 'graphql-gen-checker';
}

public function run() {
$test_results = array();

foreach ($this->getPaths() as $file) {
$schema_filename = substr($file, 0, -8).'.d.ts';
$test_results[] = $this->checkFile($file, $schema_filename, 'To regenerate, run src/cloud/api/controller/schema/update.sh');
}

return $test_results;
}
}
58 changes: 58 additions & 0 deletions arc_addons/pixielabs/unit/ArcanistProtoGenCheckerTestEngine.php
@@ -0,0 +1,58 @@
<?php

final class ArcanistProtoGenCheckerTestEngine extends ArcanistBaseGenCheckerTestEngine {
public function getEngineConfigurationName() {
return 'proto-gen-checker';
}

private function isGRPCWebProto($file) {
$proto_dir = dirname($file);
$bazel_source = '//'.$proto_dir.':'.basename($file);

list($err, $stdout) = exec_manual('bazel query --noshow_progress "kind(pl_grpc_web_library, %s/...)"', $proto_dir);
if ($err) {
return false;
}
$targets = phutil_split_lines($stdout, false);

foreach ($targets as $target) {
list($err, $stdout) = exec_manual('bazel query "kind(\'source file\', deps(%s)) intersect %s"', $target, $bazel_source);
if ($err) {
return false;
}
return count(phutil_split_lines($stdout)) > 0;
}

return false;
}

public function run() {
$test_results = array();

foreach ($this->getPaths() as $file) {
$pb_filename = substr($file, 0, -6).'.pb.go';
$test_results[] = $this->checkFile($file, $pb_filename, 'To regenerate, run: '.
'scripts/update_go_protos.sh');

if ($this->isGRPCWebProto($file)) {
// Check generated files exist. We assume they are all in src/ui/src/types/generated for now.
$fname = substr($file, strrpos($file, '/') + 1, -6);

// TODO(nick): Not all of these are in use in the main UI code anymore. Only check for the ones we need.
// Check $fname_pb.d.ts.
$test_results[] = $this->checkFile($file, 'src/ui/src/types/generated/'.$fname.'_pb.d.ts', 'To regenerate, build the grpc_web target and move the files to the correct directory');
// Check $fname_pb.js.
$test_results[] = $this->checkFile($file, 'src/ui/src/types/generated/'.$fname.'_pb.js', 'To regenerate, build the grpc_web target and move the files to the correct directory');
// Check $fname_pb.d.ts in the pixie-api package.
$test_results[] = $this->checkFile($file, 'src/ui/packages/pixie-api/src/types/generated/'.$fname.'_pb.d.ts', 'To regenerate, build the grpc_web target and move the files to the correct directory');
// Check $fname_pb.js in the pixie-api package.
$test_results[] = $this->checkFile($file, 'src/ui/packages/pixie-api/src/types/generated/'.$fname.'_pb.js', 'To regenerate, build the grpc_web target and move the files to the correct directory');
// Check $fnameServiceClientPb.ts.
// TODO(michelle): Figure out a way to make this check smarter for non-grpc protos.
// $test_results = $this->checkFile($file, 'src/ui/src/types/generated/' . ucfirst($fname) . 'ServiceClientPb.ts', 'To regenerate, build the grpc_web target and move the files to the correct directory');
}
}

return $test_results;
}
}

0 comments on commit d283511

Please sign in to comment.