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

Syscall mock testing #8

Merged
merged 6 commits into from
Oct 22, 2020
Merged

Syscall mock testing #8

merged 6 commits into from
Oct 22, 2020

Conversation

milseman
Copy link
Contributor

Syscall mock testing

Adds mocking and tracing infrastructure, test
infrastructure, and trace-based tests.

Adds mocking and tracing infrastructure, test
infrastructure, and trace-based tests.
@milseman milseman requested a review from lorentey October 21, 2020 16:53
@milseman
Copy link
Contributor Author

@lorentey I think this is ready to merge after your review. Could you make sure I'm doing sane things with Unmanaged?

@milseman milseman mentioned this pull request Oct 21, 2020
Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

I have three concerns, but overall big thumbs up:

  • [trivial] The mocking drivers are getting leaked due to a missing release
  • [technical objection] The mocking infrastructure should not be present in the binaries downstream packages/apps link with
  • [policy/process] A vague worry that this subsystem may become a barrier to contributing to this package.

Sources/SystemInternals/Mocking.swift Outdated Show resolved Hide resolved
Tests/SystemTests/TestingInfrastructure.swift Show resolved Hide resolved
Package.swift Outdated
dependencies: ["CSystem"]),
dependencies: ["CSystem"],
swiftSettings: [
.define("ENABLE_MOCKING")
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this be conditional on the current build configuration? (Or, if that's even possible, only enabled in a special test target dependency, and never in the production library?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The best I can do, IIUC, is:
.define("ENABLE_MOCKING", .when(configuration: .debug))

We also need executables as tests to build in debug and release at some point.

Copy link
Member

Choose a reason for hiding this comment

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

It's a rather unfortunate choice, but we also have the option to build & run tests outside SPM's limitations. (swift-atomics will likely also need to do that to be able to run lit tests for codegen verification.)

In this sort of setup, the test environment would build the package with ENABLE_MOCKING enabled on the swiftpm command line, enabling both the mocking infrastructure and the mocking tests themselves.

Sources/SystemInternals/Mocking.swift Show resolved Hide resolved
Sources/SystemInternals/Mocking.swift Show resolved Hide resolved
Sources/SystemInternals/Mocking.swift Outdated Show resolved Hide resolved
Tests/SystemTests/TestingInfrastructure.swift Show resolved Hide resolved
Tests/SystemTests/TestingInfrastructure.swift Outdated Show resolved Hide resolved
@milseman
Copy link
Contributor Author

@lorentey I've only enabled mocking in debug builds (best I can do to approximate testing), and I've conditionally compiled out all of the types and most of the infrastructure and use of the infrastructure. The mock_foo internal methods are still "present", but they're only called after a constant-foldable false branch and should be eliminated at compilation time.

@lorentey
Copy link
Member

I don't think it's a good idea to rely on the optimizer to eliminate these -- the mocking entry points introduce a level of userspace dynamism that feels inappropriate for a low-level syscall wrapper. Why not compile these out entirely?

@milseman
Copy link
Contributor Author

I'm normally the poster child for not trusting the optimizer to do much of anything, but trivial dead code elimination is something I'd trust the optimizer to do. The downside of conditionally compiling out the trivial functions would be that the already-onerous bodies of the system_foo calls would have even more stuff in them. The only ways I could think of around this involved helper function taking closures, which would be relying on the optimizer to do something less trivial than DCE.

FWIW, I disassembled the .o file and these mocking calls are not present.

@milseman
Copy link
Contributor Author

I think I found a way that does both conditional compilation and reduces the boilerplate. It would make it harder to promote this to a user-facing feature in the future, but hopefully we will have migrated to code generation by then.

@milseman
Copy link
Contributor Author

@lorentey Could you give this another pass? I think I improved upon all of your concerns. It is true that this will add yet another concern to contributors, but it's hopefully much less than before and fairly straight forward to follow the pattern. New syscalls would need tests anyways, so there is parity between the system_foo and the MockTestCase(name: "foo"... code.

Copy link
Member

@lorentey lorentey left a comment

Choose a reason for hiding this comment

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

This looks great! 🚢

@milseman milseman merged commit d93ee62 into apple:main Oct 22, 2020
@milseman milseman deleted the mimus_polyglottos branch October 22, 2020 18:14
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