diff --git a/backend/core/layering_test.go b/backend/core/layering_test.go new file mode 100644 index 00000000000..e8b53d2e41f --- /dev/null +++ b/backend/core/layering_test.go @@ -0,0 +1,153 @@ +/* +Licensed to the Apache Software Foundation (ASF) under one or more +contributor license agreements. See the NOTICE file distributed with +this work for additional information regarding copyright ownership. +The ASF licenses this file to You under the Apache License, Version 2.0 +(the "License"); you may not use this file except in compliance with +the License. You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package core + +import ( + "errors" + "io/fs" + "os" + "os/exec" + "path/filepath" + "sort" + "strings" + "testing" +) + +const ( + modulePrefix = "github.com/apache/devlake/" + pluginPrefix = modulePrefix + "plugins/" +) + +// frameworkRoots are the packages that must stay plugin-agnostic. +// +// The dependency direction is: plugins depend on core and helpers; core, +// helpers and server never depend on a concrete plugin. Plugins reach the +// framework through the interfaces in core/plugin, and the framework reaches +// plugins only through those same interfaces, resolved by name at runtime. +// +// server/ is in the list because it is the application, not the framework: it +// may link the generated registry (see allowedEdges) but must never reach for an +// individual plugin. +// +// The rule matters more now that plugins are compiled into the server binary by +// default: one framework→plugin import links that plugin, and everything it +// imports, into the core payload, defeats subset builds, and creates an import +// cycle for any plugin that needs the framework package doing the importing. +var frameworkRoots = []string{ + modulePrefix + "core/...", + modulePrefix + "helpers/...", + modulePrefix + "server/...", +} + +// allowedEdges holds the only framework→plugin edges that are tolerated, +// keyed by the exact importing package. Every entry is a layering exception +// that has to be argued for; keep this map as close to empty as possible. +var allowedEdges = map[string]string{ + // The composition root links the generated plugin registry, which + // blank-imports every plugin. It is the one place that is allowed to know + // that plugins exist as concrete packages. It must never import a single + // plugin directly, and no other package under server/ gets the same + // privilege. + modulePrefix + "server": modulePrefix + "plugins/all", +} + +// TestFrameworkDoesNotImportPlugins fails when anything under core/, helpers/ or +// server/ imports a plugin package, including from _test.go files. +// +// Tests count because a test that needs a plugin is a design smell: it usually +// means the framework helper under test is only exercised through one concrete +// plugin's fixtures. Move the test next to that plugin instead, or give the +// helper a plugin-independent fixture. +func TestFrameworkDoesNotImportPlugins(t *testing.T) { + args := []string{ + "list", + "-f", `{{.ImportPath}}|{{join .Imports " "}}|{{join .TestImports " "}}|{{join .XTestImports " "}}`, + } + args = append(args, frameworkRoots...) + + out, err := exec.Command("go", args...).Output() + if err != nil { + var exitErr *exec.ExitError + if errors.As(err, &exitErr) { + t.Fatalf("go list failed: %v\n%s", err, exitErr.Stderr) + } + t.Fatalf("go list failed: %v", err) + } + + seen := map[string]struct{}{} + for _, line := range strings.Split(strings.TrimSpace(string(out)), "\n") { + fields := strings.Split(line, "|") + if len(fields) != 4 { + t.Fatalf("unexpected go list output line %q", line) + } + importer := fields[0] + for _, group := range fields[1:] { + for _, imported := range strings.Fields(group) { + if !strings.HasPrefix(imported, pluginPrefix) { + continue + } + if allowedEdges[importer] == imported { + continue + } + seen[importer+" imports "+imported] = struct{}{} + } + } + } + + if len(seen) == 0 { + trackFrameworkSources(t) + return + } + + violations := make([]string, 0, len(seen)) + for v := range seen { + violations = append(violations, v) + } + sort.Strings(violations) + + t.Fatalf("the framework must not import plugins, found %d edge(s):\n\t%s", + len(violations), strings.Join(violations, "\n\t")) +} + +// trackFrameworkSources registers every framework source file with the test log. +// +// The test cache only knows about the package under test, not about the other +// packages this test inspects through `go list`. Without this, a plain +// `go test ./core` would replay a cached PASS after someone adds a plugin +// import in helpers/ or server/. Opening the files makes them part of the cache +// entry, so any change to them invalidates it. +func trackFrameworkSources(t *testing.T) { + t.Helper() + + out, err := exec.Command("go", "env", "GOMOD").Output() + if err != nil { + return + } + moduleDir := filepath.Dir(strings.TrimSpace(string(out))) + + for _, dir := range []string{"core", "helpers", "server"} { + _ = filepath.WalkDir(filepath.Join(moduleDir, dir), func(path string, d fs.DirEntry, err error) error { + if err != nil || d.IsDir() || !strings.HasSuffix(path, ".go") { + return nil + } + // Best effort: the cache entry only needs the file to be opened. + _, _ = os.ReadFile(path) + return nil + }) + } +} diff --git a/backend/helpers/e2ehelper/data_flow_tester_test.go b/backend/helpers/e2ehelper/data_flow_tester_test.go index e3dd72530c9..75f3b2023e0 100644 --- a/backend/helpers/e2ehelper/data_flow_tester_test.go +++ b/backend/helpers/e2ehelper/data_flow_tester_test.go @@ -19,10 +19,9 @@ package e2ehelper import ( "github.com/apache/devlake/core/dal" + "github.com/apache/devlake/core/errors" "github.com/apache/devlake/core/models/common" plugin "github.com/apache/devlake/core/plugin" - gitlabModels "github.com/apache/devlake/plugins/gitlab/models" - "github.com/apache/devlake/plugins/gitlab/tasks" "testing" "github.com/stretchr/testify/assert" @@ -39,29 +38,48 @@ func (t TestModel) TableName() string { return "_tool_test_model" } +type TestOptions struct { + ProjectId int +} + +type TestTaskData struct { + Options *TestOptions +} + +// testExtractIssuesMeta stands in for a real plugin's subtask so this example +// stays self-contained: helpers must not import a concrete plugin. +var testExtractIssuesMeta = plugin.SubTaskMeta{ + Name: "extractIssues", + EntryPoint: func(taskCtx plugin.SubTaskContext) errors.Error { + return nil + }, +} + func ExampleDataFlowTester() { var t *testing.T // stub - var gitlab plugin.PluginMeta - dataflowTester := NewDataFlowTester(t, "gitlab", gitlab) + var pluginMeta plugin.PluginMeta + dataflowTester := NewDataFlowTester(t, "test_plugin", pluginMeta) - taskData := &tasks.GitlabTaskData{ - Options: &tasks.GitlabOptions{ + taskData := &TestTaskData{ + Options: &TestOptions{ ProjectId: 666888, }, } // import raw data table - dataflowTester.ImportCsvIntoRawTable("./tables/_raw_gitlab_api_issues.csv", "_raw_gitlab_api_issues") + dataflowTester.ImportCsvIntoRawTable("./tables/_raw_test_plugin_api_issues.csv", "_raw_test_plugin_api_issues") // verify extraction - dataflowTester.FlushTabler(gitlabModels.GitlabProject{}) - dataflowTester.Subtask(tasks.ExtractApiIssuesMeta, taskData) + dataflowTester.FlushTabler(&TestModel{}) + dataflowTester.Subtask(testExtractIssuesMeta, taskData) dataflowTester.VerifyTable( - gitlabModels.GitlabIssue{}, - "tables/_tool_gitlab_issues.csv", + &TestModel{}, + "tables/_tool_test_model.csv", []string{ - "gitlab_id", + "connection_id", + "issue_id", + "label_name", "_raw_data_params", "_raw_data_table", "_raw_data_id", @@ -108,10 +126,14 @@ func TestGetTableMetaData(t *testing.T) { fields, err := dal.GetPrimarykeyColumnNames(dataflowTester.Dal, &TestModel{}) assert.Equal(t, err, nil) assert.Equal(t, 3, len(fields)) + // GetPrimarykeyColumnNames qualifies each column with the table name, so + // the result can be used as-is in join conditions whose column names + // collide with reserved identifiers. Callers that want the bare column + // name strip the prefix themselves. for _, e := range []string{ - "connection_id", - "issue_id", - "label_name", + "_tool_test_model.connection_id", + "_tool_test_model.issue_id", + "_tool_test_model.label_name", } { assert.Contains(t, fields, e) } diff --git a/backend/server/services/blueprint_makeplan_v200_test.go b/backend/server/services/blueprint_makeplan_v200_test.go index 161f95ed91e..f2542a77a68 100644 --- a/backend/server/services/blueprint_makeplan_v200_test.go +++ b/backend/server/services/blueprint_makeplan_v200_test.go @@ -27,7 +27,6 @@ import ( "github.com/apache/devlake/core/models/domainlayer/ticket" "github.com/apache/devlake/core/plugin" mockplugin "github.com/apache/devlake/mocks/core/plugin" - "github.com/apache/devlake/plugins/org/tasks" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" ) @@ -71,9 +70,17 @@ func TestMakePlanV200(t *testing.T) { // mock org plugin org := new(mockplugin.CompositeProjectMapper) + // The org plugin normally builds a tasks.ProjectMapping here. This test + // mocks the plugin, so the value only has to round-trip through the plan: + // keep it local rather than importing plugins/org/tasks, which would make + // the framework depend on a plugin. + projectMapping := map[string]interface{}{ + "projectName": projectName, + "scopes": []interface{}{}, + } orgPlan := coreModels.PipelinePlan{ { - {Plugin: "org", Subtasks: []string{"setProjectMapping"}, Options: map[string]interface{}{"projectMappings": []interface{}{tasks.NewProjectMapping(projectName, githubOutputScopes)}}}, + {Plugin: "org", Subtasks: []string{"setProjectMapping"}, Options: map[string]interface{}{"projectMappings": []interface{}{projectMapping}}}, }, } org.On("MapProject", projectName, githubOutputScopes).Return(orgPlan, nil)