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

instead of depending on test cases from clvm and clvm_tools #177

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

arvidn
Copy link
Contributor

@arvidn arvidn commented Mar 1, 2022

rely on the existing unit tests in src/test_ops.rs

This is a step towards untangling clvm_rs from clvm and clvm_tools, which will allow us to change clvm_tools's brun to report accurate costs without breaking the tests in this repository.

@arvidn arvidn requested a review from richardkiss March 1, 2022 15:39
@richardkiss
Copy link
Contributor

Taking out the clvm_tools tests makes perfect sense, since the test results are ignored due to stage 2 not working with clvm_rs. I do have concerns about taking out the clvm tests permanently. We could for now create a branch of clvm specifically to be imported by clvm_rs (maybe call it clvm_rs_tests).

@arvidn
Copy link
Contributor Author

arvidn commented Mar 1, 2022

right. running the clvm tests still depend on clvm_tools though. I think long term, those test cases should be executed in a more direct way, that doesn't go through brun.

If I would go through and make sure all test cases in the existing .txt files are present in the test_ops.rs test cases, do you think that would be a good enough substitute?

@richardkiss
Copy link
Contributor

Sure. The more similar the test formats can be between the two, the easier it will be to keep the two in sync if we add more tests or change existing ones. So I'd recommend trying to keep the same text input/output format, maybe even the exact same files, reading them at runtime.

@arvidn
Copy link
Contributor Author

arvidn commented Mar 2, 2022

I don't see the value in tying clvm_rs together with clvm. We don't really need to ensure they move in lock step anymore, do we? e.g. the op_div softfork hasn't been implemented in clvm yet, nor is it tested by those test cases.

To me it seems it would only keep us bogged down, slowing down updates to clvm_rs and clvm_tools.

Had they been part of the same repository, and changes could be made to all 3 projects in single commits, it would have been different, I think. At least simpler.

@github-actions
Copy link

github-actions bot commented May 1, 2022

'This PR has been flagged as stale due to no activity for over 60
days. It will not be automatically closed, but it has been given
a stale-pr label and should be manually reviewed.'

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants