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

Introduce MockVM #1049

Merged
merged 15 commits into from
Dec 18, 2023
Merged

Introduce MockVM #1049

merged 15 commits into from
Dec 18, 2023

Conversation

qinsoon
Copy link
Member

@qinsoon qinsoon commented Dec 14, 2023

This PR introduces MockVM, a type that implements VMBinding and allows users to control the behavior of each method for testing. This PR also moves all the tests in the current DummyVM to MockVM, and removes the current DummyVM.

This PR closes #99. Note that the current MockVM implementation does not allow changing constants or associated types in VMBinding -- I would suggest we create another issue to track this problem.

Changes:

  • Introduce MockVM, and write all the current DummyVM tests with MockVM.
  • Remove DummyVM, and remove ./examples which uses DummyVM.
  • Change CI scripts to run tests with MockVM.
  • Remove pub visibility for some modules. Those modules were exposed so we can test them from DummyVM. As now MockVM is a part of the main crate, we can test private items. We no longer need pub for those modules.

@qinsoon qinsoon added the PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead) label Dec 14, 2023
@qinsoon qinsoon marked this pull request as ready for review December 14, 2023 12:52
@qinsoon qinsoon requested a review from wks December 14, 2023 12:52
Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

I think the MockAny may not work because the original closures are supposed to accept objects that are implemented with different type arguments. But this fact still doesn't prevent us from doing mock tests for those methods. I have given concrete examples in in-line comments. For the root-scanning methods, we simply let the closure provide the list of roots instead of making a dyn callback objects from it to call. Similarly, for the weak reference processing methods, we just let the closure list the object references to be traced.

However, I tried to write a mock test case for testing the ScanVMSpecificRoots work packet, but failed because I found it hard to instantiate a concrete ProcessEdgesWorkRootWorkFactory without knowing the plan.

src/util/test_util/mock_vm.rs Outdated Show resolved Hide resolved
src/util/test_util/mock_vm.rs Outdated Show resolved Hide resolved
src/util/test_util/mock_vm.rs Show resolved Hide resolved
src/util/test_util/mock_vm.rs Show resolved Hide resolved
benches/alloc.rs Outdated Show resolved Hide resolved
benches/sft.rs Outdated Show resolved Hide resolved
src/util/metadata/header_metadata.rs Show resolved Hide resolved
docs/userguide/src/portingguide/perf_tuning/alloc.md Outdated Show resolved Hide resolved
src/util/test_util/mock_vm.rs Show resolved Hide resolved
Copy link
Collaborator

@wks wks left a comment

Choose a reason for hiding this comment

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

LGTM

@qinsoon qinsoon added this pull request to the merge queue Dec 18, 2023
Merged via the queue into mmtk:master with commit 658bce8 Dec 18, 2023
19 of 20 checks passed
@qinsoon qinsoon deleted the mock-vm branch December 18, 2023 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR-testing Run binding tests for the pull request (deprecated: use PR-extended-testing instead)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

MockVM
2 participants