Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add v2 modules to integration tests #2682

Merged
merged 9 commits into from
Dec 29, 2023
Merged

Conversation

doriable
Copy link
Member

@doriable doriable commented Dec 20, 2023

This adds v2 modules to all buf/cmd/buf workspace and lint integration tests.
It also covers a couple of a bit of clean-up around error messages.

Copy link
Member

@bufdev bufdev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would love some tests added to probably buflint_test.go that test the new ignore_only paths being relative to the buf.yaml instead of being root-relative, and some tests that test excludes per directory in these integration tests, does that make sense/could you add those?

@bufdev
Copy link
Member

bufdev commented Dec 21, 2023

Also, are there any tests checking how v2 integrates with --config? I already can't remember the design decision I made regarding --config with inline v2 values, but would love us to lock in that behavior.

@doriable doriable changed the title Add v2 modules to buf/cmd/buf integration tests Add v2 modules to integration tests Dec 27, 2023
@doriable
Copy link
Member Author

Also, are there any tests checking how v2 integrates with --config? I already can't remember the design decision I made regarding --config with inline v2 values, but would love us to lock in that behavior.

So there currently is a test that inlines v2 config with --config. It seems that the expectation right now is that it returns a non-zero status code (1) but does not provide a helpful error string.

testRunStdout(
t,
nil,
1,
"",
"lint",
filepath.Join("testdata", "workspace", "success", baseDirPath),
"--config",
`version: v2
modules:
- directory: a
- directory: other/proto
lint:
use:
- PACKAGE_DIRECTORY_MATCH
- directory: proto`,
"--path",
filepath.Join("testdata", "workspace", "success", baseDirPath, "other", "proto", "request.proto"),
)

@doriable doriable requested a review from bufdev December 28, 2023 21:14
@@ -1109,30 +1168,40 @@ func testLintWithOptions(
storageos.ReadWriteBucketWithSymlinksIfSupported(),
)
require.NoError(t, err)
var options []bufworkspace.WorkspaceBucketOption
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This isnt used

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yes, I ended up going with targeting the opaque IDs instead

)
require.NoError(t, err)

if moduleFullNameString == "" {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to:

opaqueID := moduleFullNameString
if opaqueID == "" {
  opaqueID = "."
}

@@ -190,6 +190,40 @@ func ModuleSetToDAG(moduleSet ModuleSet) (*dag.Graph[string], error) {
return graph, nil
}

// ModuleToSelfContainedModuleReadBucketWithOnlyProtoFiles converts the Module to a
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this move?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I think I moved this by accident while traversing some of the helpers. Moved it back.

@bufdev bufdev merged commit 3d15824 into bufmod Dec 29, 2023
6 of 7 checks passed
@bufdev bufdev deleted the BSR-3130-v2-workspace-tests branch December 29, 2023 00:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants