GODRIVER-2724 Move internal-only packages to "internal/" directories #1486
Conversation
API Change Report./benchmarkincompatible changespackage removed ./cmd/build-oss-fuzz-corpusincompatible changespackage removed ./cmd/godriver-benchmarkincompatible changespackage removed ./cmd/parse-api-reportincompatible changespackage removed ./cmd/testatlasincompatible changespackage removed ./cmd/testawsincompatible changespackage removed ./cmd/testentauthincompatible changespackage removed ./cmd/testkmsincompatible changespackage removed ./examples/documentation_examplesincompatible changespackage removed ./mongo/integrationincompatible changespackage removed |
prestonvasquez
requested review from
matthewdale
and removed request for
a team
There was a problem hiding this comment.
Do we need to update the oss-fuzz integration when we make this change?
There was a problem hiding this comment.
We will need to create a mongo-v2 project on the oss-fuzz repo, the ticket for this is here: https://jira.mongodb.org/browse/GODRIVER-3064
I don't think we need to make a change in our repository. This line should change on the oss-fuzz repo to
go run internal/test/build-oss-fuzz-corpus/main.go $OUT/fuzz_decode_seed_corpus.zip
internal/test/benchmark/main.go
Outdated
There was a problem hiding this comment.
The directory organization is inconsistent between main packages. For example, this package is in internal/test, but other main packages are in internal/test/cmd.
Conventionally, main package Go files are in a path like cmd/[binary name]/[Go files]. In this case, we're putting these in an internal directory, so the directory structure should be internal/cmd/[binary name]/[Go files].
There was a problem hiding this comment.
I think I agree with this. I originally separated it because we don't actually compile a usable binary from godriver-benchmark, as we do with all of the other package in cmd. We compile and execute it using go run, here.
What are your thoughts?
There was a problem hiding this comment.
go run is just a shortcut for "compile a binary and then run it", so it is fundamentally the same thing. A main package is always built and run as an executable binary, so main packages should always be in an internal/cmd directory.
There was a problem hiding this comment.
Optional: The test directory seems redundant here. Consider moving it to internal/integration or the existing internal/integtest package.
There was a problem hiding this comment.
I am fine with either solution, but it is kind of nice to seperate code that is intended to test the driver against logic depended on by the driver.
There was a problem hiding this comment.
Optional: The integration package really has two kinds of code:
- Integration tests for the driver in the
_test.gofiles. - Integration test logic, including the unified spec test runner, in the non-
_test.gofiles.
One option is to move all of the _test.go files into the mongo directory and change the package declaration to package mongo_test (instead of package mongo) so that they don't have access to unexported logic. You can optionally rename them to _integ_test.go or something similar to indicate they are integration tests. That would localize all tests to the same directory so they're not as scattered around.
There was a problem hiding this comment.
I've create GODRIVER-3066 to followup on this.
There was a problem hiding this comment.
Moving the unified spec test runner logic into an internal directory will break the integration in drivers-atlas-testing. We'll need a follow-up change to fix that integration once this is merged.
There was a problem hiding this comment.
Oh interesting, good catch. I've created GODRIVER-3065 to address this.
GODRIVER-2724
Summary
Limit the surface area of the stable API by moving packages to the internal directory.
Background & Motivation
From the ticket description: