This is an automated email from the ASF dual-hosted git repository.
Startrekzky pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/devlake.git
The following commit(s) were added to refs/heads/main by this push:
new 2f3d07ea9 test(core): guard the framework against plugin imports
(#9277)
2f3d07ea9 is described below
commit 2f3d07ea9dc86608551506b0ea5b42e0db6713f2
Author: Klesh Wong <[email protected]>
AuthorDate: Fri Oct 30 15:13:46 2026 +0800
test(core): guard the framework against plugin imports (#9277)
* test(core): forbid the framework from importing plugin packages
The dependency direction is plugins -> core/helpers, never the reverse.
Nothing enforced it, and two test files had already crossed the line:
helpers/e2ehelper imported plugins/gitlab and server/services imported
plugins/org.
Compiling plugins into the server binary makes the rule load-bearing: a
single framework->plugin import links that plugin and its whole dependency
tree into the core payload and defeats subset builds.
Add TestFrameworkDoesNotImportPlugins, which reads the import graph of
core/, helpers/ and server/ (including _test.go files) via `go list` and
fails on any plugins/ edge. server/ may only link the generated
plugins/all registry, which is encoded as one explicit exception.
Make the two offending tests plugin-agnostic: the e2ehelper example now
uses local fixtures, and the blueprint test builds its mocked org task
option locally.
* test(e2ehelper): expect qualified primary key column names
GetPrimarykeyColumnNames has qualified every column with its table name
since 8e915cd59 ("feat: set project mapping by plugin org"), so the result
can be dropped into a join condition without colliding with a reserved
identifier. Callers that want the bare name strip the prefix themselves
(helpers/e2ehelper/data_flow_tester.go:316, and
helpers/srvhelper/scope_config_service_helper.go:74 checks the suffix).
The test still expected the unqualified names, so it had been failing. It
went unnoticed because scripts/unit-test-go.sh filters out every package
path matching `e2e`, which covers helpers/e2ehelper; the package needs a
live E2E_DB_URL, so it stays out of the DB-less unit-test job.
---------
Co-authored-by: Louis.z <[email protected]>
---
backend/core/layering_test.go | 153 +++++++++++++++++++++
backend/helpers/e2ehelper/data_flow_tester_test.go | 52 +++++--
.../services/blueprint_makeplan_v200_test.go | 11 +-
3 files changed, 199 insertions(+), 17 deletions(-)
diff --git a/backend/core/layering_test.go b/backend/core/layering_test.go
new file mode 100644
index 000000000..e8b53d2e4
--- /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 e3dd72530..75f3b2023 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 161f95ed9..f2542a77a 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)
