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

Debug mode via cli option #1502

Merged
merged 11 commits into from
Sep 24, 2024

Conversation

yasonk
Copy link

@yasonk yasonk commented Sep 17, 2024

Describe your changes

This will introduce --debug flag to the run command in the CLI as described in issue #1322.

Includes the following tangental changes:

  1. Minor typo fixes in the docs touched.
  2. Makefile to support testing a single package.
  3. Updates to existing code for consistency (e.g. using StackInputs::default() instead of Default::default())

Issues found
The unit tests in the miden-vm package don't run when executing make test. Documented in the issue #1501. Ran the unit tests manually.

Checklist before requesting a review

  • Repo forked and branch created from next according to naming convention.
  • Commit messages and codestyle follow conventions.
  • Relevant issues are linked in the PR description.
  • Tests added for new functionality.
  • Documentation/comments updated according to changes.

Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for doing this! This is not a full review, but I left a few comments inline.

processor/src/lib.rs Outdated Show resolved Hide resolved
processor/src/lib.rs Outdated Show resolved Hide resolved
assembly/src/assembler/instruction/mod.rs Outdated Show resolved Hide resolved
Comment on lines +68 to +70
.PHONY: test-package
test-package: ## Tests specific package: make test-package package=miden-vm
$(DEBUG_ASSERTIONS) cargo nextest run --cargo-profile test-release --features testing -p $(package)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice! A question: would we achieve the same result if we add executable feature? e.g.,:

$(DEBUG_ASSERTIONS) cargo nextest run --cargo-profile test-release --features testing,executable

Copy link
Author

Choose a reason for hiding this comment

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

The thing that I added will run the integration tests for just one package as a convenience to avoid having to run all packages. Adding feature "executable" is required to run the unit tests inside the binary crate of miden-vm package. I created issue #1501 with the question regarding whether adding "executable" is the right way fix the problem of unit tests in miden-vm not running when using make tests

miden/src/cli/run.rs Outdated Show resolved Hide resolved
miden/src/cli/run.rs Outdated Show resolved Hide resolved
miden/src/cli/run.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

Looks great! Thank you! I left a couple more small comments inline.

CHANGELOG.md Outdated Show resolved Hide resolved
miden/src/cli/run.rs Outdated Show resolved Hide resolved
miden/src/cli/run.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

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

All looks good! Thank you! I fixed the merge conflict and will merge as soon as CI is green.

@bobbinth bobbinth merged commit c6e2bc8 into 0xPolygonMiden:next Sep 24, 2024
9 checks passed
@yasonk yasonk deleted the debug-mode-via-cli-option branch September 24, 2024 22:12
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