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

Jump to bottom

runtime: clean up export_test.go #55108

Open
aclements opened this issue Sep 16, 2022 · 4 comments
Open

runtime: clean up export_test.go #55108

aclements opened this issue Sep 16, 2022 · 4 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Thinking
Milestone
Backlog

Comments

@aclements
Copy link
Member

@mknyszek, @cherrymui, @rsc, and I were discussing how to simplify runtime/export_test.go. It's become quite ungainly, and the difficulty of re-exporting runtime internals (especially data structures) I believe actively discourages us from writing unit tests. There are a few things we could do:

  1. Generate export_test.go. Have some way of describing what runtime internals we want and just code generate the bulk of the wrappers. This is easy for functions, and remains messy for data structures, but at least a tool would be handling the mess.
  2. Flip things around. Put (most) runtime unit tests in package runtime instead of runtime_test, so they can directly access runtime internals. Now because of layering we have the reverse problem we do today. These test functions can't import testing, so we would copy the testing.TB interface into runtime, write the tests to take this interface, and autogenerate trivial TestX(*testing.T) functions into runtime_test that would just call the real tests. We would also need access to some of the standard library (e.g., sync.WaitGroup, reflect.DeepEqual), and for that we can inject closures into the runtime test. The key advantage of this approach is that rather than trying to access unexported things across packages, we're accessing exported things across packages.
  3. Make it possible to import testing from runtime. The testing package already widely uses the sort of cross-DAG injection I described above to minimize its dependencies, but it could go even further. This would benefit a couple dozen packages that can't import testing today. The advantage of this is that we wouldn't need to generate TestX wrappers because we could use testing.T directly. However, we would still need to inject other library dependencies as in option 2. Since it's easy to write a tool to generate the wrappers and this tool could be shared, it's not clear this is much of an advantage.

Overall, it seems like option 2 is the best balance. We can also migrate toward it incrementally, perhaps writing new tests in this style, or moving particularly onerous export_test tests over.
@aclements aclements added Thinking compiler/runtime Issues related to the Go compiler and/or runtime. labels Sep 16, 2022
@aclements aclements added this to the Backlog milestone Sep 16, 2022
@aclements
Copy link
Member Author

cc @golang/runtime

@cherrymui cherrymui added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Sep 19, 2022
@bcmills
Copy link
Contributor

bcmills commented Feb 6, 2023

As a fourth option, would it be possible to factor out the shared internals into a package within runtime/internal? At least in theory, both the runtime package and the runtime_test package could import that.

(But maybe that's not feasible because too much of the runtime would have to move to the internal package along with it?)

@gopherbot
Copy link

Change https://go.dev/cl/466096 mentions this issue: runtime: create an API for unwinding inlined frames

@aclements
Copy link
Member Author

As a fourth option, would it be possible to factor out the shared internals into a package within runtime/internal?

Thanks for bringing that up. I've thought about this too. I suspect we'd have to move most of the runtime into an internal package for this to work. We could, in effect, make the runtime package just be the public runtime API, which is not very big, and move the whole runtime implementation somewhere else. That doesn't feel worth it for this.

Long ago, Russ and Alan did an experiment with analyzing the runtime call graph to see if we could break it into smaller packages and the answer was basically "maybe, but it would take a lot of work". That work seems worth doing incrementally, which we've sort of been doing.

gopherbot pushed a commit that referenced this issue Mar 10, 2023
@aclements
runtime: create an API for unwinding inlined frames
f52bede 
We've replicated the code to expand inlined frames in many places in
the runtime at this point. This CL adds a simple iterator API that
abstracts this out.

We also use this to try out a new idea for structuring tests of
runtime internals: rather than exporting this whole internal data type
and API, we write the test in package runtime and import the few bits
of std we need. The idea is that, for tests of internals, it's easier
to inject public APIs from std than it is to export non-public APIs
from runtime. This is discussed more in #55108.

For #54466.

Change-Id: Iebccc04ff59a1509694a8ac0e0d3984e49121339
Reviewed-on: https://go-review.googlesource.com/c/go/+/466096
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Michael Pratt <mpratt@google.com>
Run-TryBot: Austin Clements <austin@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. Thinking
Projects
Go Compiler / Runtime
Status: No status
Milestone
Backlog
Development

No branches or pull requests
4 participants
@aclements @bcmills @gopherbot @cherrymui