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

implement RhoSpec contract in Rholang #2100

Merged
merged 1 commit into from Feb 4, 2019

Conversation

Projects
None yet
5 participants
@odeac
Copy link
Collaborator

odeac commented Jan 11, 2019

Overview

  • add clues in the assertions so we can differentiate between different failures
  • fix the names and URNs of the assert primitives
  • migrate rholang tests to RhoSpec
  • remove the tests for BasicWallet and MakePoS as they are not easy to migrate while being irrelevant because their API will change soon
  • rename TestSetUtil to TestUtil

JIRA ticket:

https://rchain.atlassian.net/browse/RCHAIN-2292

Notes

The RhoSpec contract is just a demo of the assert primitives for now because I don't know yet how to sign and deploy a contract the way TestSet is deployed now. Once I figure that out I will move the implementation to a new file.

Please make sure that this PR:

Bors cheat-sheet:

  • bors r+ runs integration tests and merges the PR (if it's approved),
  • bors try runs integration tests for the PR,
  • bors delegate+ enables non-maintainer PR authors to run the above.

@odeac odeac requested review from JoshOrndorff , KentShikama and ArturGajowy Jan 11, 2019

@ArturGajowy
Copy link
Collaborator

ArturGajowy left a comment

I'm very sorry to say this, but...

Overall, I don't like where this is going. I'm seriously worried about the safety net the current tests provide, and thus our safety.

Why aren't we adapting the TestSet contract step-by-step so that we see the changed API is usable and better?
Why are we discarding all the old API's design choices, without even discussing them?
Why are we rewriting the test harness in the first place?
(do you realize how easy it is to paint all tests green by accident when doing that? I've been doing similar things in the past and my feeling is it's guaranteed...)

I am very much against the total reimplementation of the casper test harness, then leading to a major rewrite of most of the tests. That's an equivalent of re-weaving our safety net while we're working 5 floors above it.

If we're making any changes to the test harness - I'd like them to be incremental and introducing changes step-by-step to the current implementation.


To me, we should start with making TestSet accept comparison descriptions. So instead of:

          @TestSet!("define",
            "Wallet deposit should work as expected.",
            [
              [*walletBalance, 100],
              [*overdrawDep, false],
              [*depWallet, true],
              [*walletBalance, 130]
            ],
            *test3
          )

we'd have

          @TestSet!("define",
            "Wallet deposit should work as expected.",
            [
              ["wallet balance", *walletBalance, 100],
              ["overdraw deposit result", *overdrawDep, false],
              ["successful deposit result", *depWallet, true],
              ["wallet balance afterwards", *walletBalance, 130]
            ],
            *test3
          )

This alone should be enough to have meaningful assertion messages.


I see it's not your only goal and I think I agree or can relate with some of the choices I think you've deliberately taken. I'd like us to discuss them though before implementing them and then seeing that all the tests we have need to be rewritten from scratch without a remotely clear migration path.

Features of the TestSet API:

We should discuss them before discarding them.

  • Notice how this API minimizes noise by defining all the assertions at once.
  • It also seems to leverage rholang in a way I don't understand yet to see the two wallet balances one after another.
  • It also clearly allows for expressing time dependencies between tests and sharing state across them. Whether we like that or not, we need to provide a way for that in the new API.
  • It's undocumented. We should get to know it though and maybe document some things if they make sense instead of discarding them without discussion.

Features I think you seem to target:

  • tests independent from each other
  • tests execution controlled by the framework, not the tested code
    • so that no test gets lost due to failures in others, which seems to be the real goal here
  • specialized assertion variants
  • assertion clues

All of those should be discussed based on examples from the existing test base before implementing them. Of all of those, I think the least controversial would probably be 'specialized assertion variants'.


I guess we can do the discussion here or move to the wiki. I'm not requiring that we discuss the whole design upfront. My requests are:

  • do the changes to the current harness gradually making it better and refactoring as needed
  • discuss the goals behind each harness API change and choose the goal fulfillment strategy before implementing it.
@KentShikama

This comment has been minimized.

Copy link
Collaborator

KentShikama commented Jan 11, 2019

Why are we discarding all the old API's design choices, without even discussing them?

I don't think there was much discussion for the old API. I wouldn't treat it as a sacred battle tested piece of code.

That's an equivalent of re-weaving our safety net while we're working 5 floors above it.

Over the past couple months we've been slowly tightening our safety net. At this point, the comm layer, block proposing and merging, the estimator, and contract execution in rholang is pretty solid. Bonding and safety oracle are being tightened up. Casper contracts (including wallets, REV, proof of stake) are still WIP and in some areas still not yet completely defined. Perhaps it is a matter of style but I think we can be a bit more aggressive with casper contracts.

@ArturGajowy

This comment has been minimized.

Copy link
Collaborator

ArturGajowy commented Jan 11, 2019

I wouldn't treat it as a sacred battle tested piece of code.

I'm far from that. I'm just sure it's more battle-tested than starting from scratch.

been slowly tightening our safety net

And now we're seemingly on the course to losing it due to errors in a big-bang revamp of the net's foundations.

Who tests the tests, after all? How can we tell the tests still catch the bugs they used to after we've rewritten them?

@KentShikama

This comment has been minimized.

Copy link
Collaborator

KentShikama commented Jan 11, 2019

And now we're seemingly on the course to losing it due to errors in a big-bang revamp of the net's foundations.

To be clear these tests have nothing to do with the already solid "comm layer, block proposing and merging, the estimator, and contract execution in rholang".

@ArturGajowy

This comment has been minimized.

Copy link
Collaborator

ArturGajowy commented Jan 11, 2019

Are you then saying all the casper tests utilizing the TestSet contract are garbage and can be discarded?

@KentShikama

This comment has been minimized.

Copy link
Collaborator

KentShikama commented Jan 11, 2019

Are you then saying all the casper tests utilizing the TestSet contract are garbage and can be discarded?

Besides Either, ListOps, and NonNegativeNumber - I don't see those changing much. Depending on the outcome of the wallet and validator incentive discussions a lot of contracts could go through some drastic change.

@ArturGajowy

This comment has been minimized.

Copy link
Collaborator

ArturGajowy commented Jan 11, 2019

It's not about whether any of the production code changes or not. It's about whether our test code can still be believed to secure (against) those changes and make them controllable.

@rabbitonweb

This comment has been minimized.

Copy link
Collaborator

rabbitonweb commented Jan 11, 2019

I don't understand why there is work in this direction? @odeac haven't we had a meeting regarding this?
we agreed that the framework has issues but putting sth new will be a challange and we have other places in code we should focus on. why have we spent 1h talking, taking conculsions, if we just gonna ignore the output of the meeting?

I second @ArturGajowy, my main concerns are following:

  1. reimplementing existing test on a new contract is risky, as we might get false-positivies
  2. changing current TestSet to apply to main issues (mainly ones that are mentioned in https://rchain.atlassian.net/browse/RCHAIN-2292) seem to be doable. They will require effort but small one comparing to creating new framework and reimplmenting tests.

I'm not argueing against the feature, but timing does not seem right? You might say "well, hell, but the work is done, what's the problem?". my issue is that the work is not completed and there will be another huge effort moving this framework onto the existing test suite. or do you want to keep both?

@KentShikama

This comment has been minimized.

Copy link
Collaborator

KentShikama commented Jan 11, 2019

@ArturGajowy @rabbitonweb I think we are overestimating what is at stake here. This isn't like PR #1831 where we are about to change 3000 lines on areas of code that have gone through 3~4 months of community testing. We've barely tested or even agreed on the final designs of the wallet contracts. Validator incentive contracts are still WIP. Worst case we're only losing the tests that were written for Either, ListOps, and NonNegativeNumber.

@ArturGajowy

This comment has been minimized.

Copy link
Collaborator

ArturGajowy commented Jan 11, 2019

So you're saying that of

MakePoSTest.rho
BasicWalletTest.rho
MakeMintTest.rho
EitherTest.rho
NonNegativeNumberTest.rho

we can discard all but the last two?

@KentShikama

This comment has been minimized.

Copy link
Collaborator

KentShikama commented Jan 11, 2019

I wouldn't just discard them but their value isn't that large given how we will change the corresponding contracts.

@ArturGajowy

This comment has been minimized.

Copy link
Collaborator

ArturGajowy commented Jan 11, 2019

Still, the new framework lacks usage examples (not a single test was ported so far) and it's clear porting won't be straightforward due to conflicting design paradigms ('all tests must be isolated' vs 'tests reuse the previous state and can be sequenced by the user'). While clean-slate design has its merits, it only works after identifying the use cases and requirements - and verifying them by examples.

If this is to be useful, it needs a battle test of porting all the existing tests. Given what you said @KentShikama, I'm now comfortable if this means rewriting them largely. But this should be done before we deem the new framework ready. Otherwise the solution is incomplete and only increases maintenance burden.

@JoshOrndorff JoshOrndorff removed their request for review Jan 12, 2019

@dckc

This comment has been minimized.

Copy link
Collaborator

dckc commented Jan 16, 2019

@rabbitonweb writes:

why have we spent 1h talking, taking conculsions, if we just gonna ignore the output of the meeting?

I'm interested to look at the record of the meeting. Pointer, please?

If there is no record, well, that answers your question.

@odeac odeac added the do-not-merge label Jan 17, 2019

@odeac

This comment has been minimized.

Copy link
Collaborator Author

odeac commented Jan 17, 2019

@dckc The discussion @rabbitonweb was referring to was about my proposal to have a Rholang unit testing framework which would be used by the dapp developers.

This PR is about making the Casper Rholang tests easier to debug by reporting insights about why the tests failed because, without such information, it's very difficult to figure out why they failed.

@dckc dckc referenced this pull request Jan 22, 2019

Open

Standard for testing #6

@odeac odeac removed the do-not-merge label Jan 24, 2019

@odeac odeac force-pushed the odeac:rhol-2292-part3 branch 2 times, most recently from 63470b2 to 05a9018 Jan 24, 2019

@odeac odeac referenced this pull request Jan 25, 2019

Merged

purse security: mutual authentication (RCHAIN-2559) #2140

5 of 5 tasks complete
@ArturGajowy

This comment has been minimized.

Copy link
Collaborator

ArturGajowy commented Jan 28, 2019

@odeac please reinstate a proper commit log. This PR is not reviewable otherwise, just like any other PR having commits bigger than at most 200 lines of code (excluding tests) (which we require for PRs, and then even more so for all their commits)

@ArturGajowy
Copy link
Collaborator

ArturGajowy left a comment

A couple comments, but this looks mostly good.

I didn't check that tests where migrated faithfully, but they look ok and visibly more readable 👍

The 'only concrete terms should be allowed in comparison' comment seems important, as I've no idea what's going to happen if s/b passes a term that's using variables. Neither do the tests.

Please rebase and apply comments. Please also squash the 'remove commented code' commit.

@odeac odeac force-pushed the odeac:rhol-2292-part3 branch 2 times, most recently from aad29fd to 7eb557d Feb 1, 2019

@ArturGajowy
Copy link
Collaborator

ArturGajowy left a comment

Please rebase and squash all the fixups into the resulting single commit, and we're good to merge.

@odeac

This comment has been minimized.

Copy link
Collaborator Author

odeac commented Feb 4, 2019

bors r+

bors bot added a commit that referenced this pull request Feb 4, 2019

Merge #2100 #2116 #2157
2100: implement RhoSpec contract in Rholang r=odeac a=odeac



2116: Modify MultiParentCasperRef to return default closed over effect r=rabbitonweb a=rabbitonweb



2157: Rchain 2830: add command line arguments for RegistrySigGen r=odeac a=odeac



Co-authored-by: Ovidiu Deac <ovidiu.deac@rchain.coop>
Co-authored-by: Pawel Szulc <paul.szulc@gmail.com>
@ArturGajowy

This comment has been minimized.

Copy link
Collaborator

ArturGajowy commented Feb 4, 2019

bors r-

@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Feb 4, 2019

Canceled

@ArturGajowy
Copy link
Collaborator

ArturGajowy left a comment

I've asked for a squash before merge. The fixup commits don't bring any vaule to the commit log.

implement RhoSpec contract in Rholang
rename TestSetUtil to TestUtil
remove unused functions from TestUtil
remove irrelevant casper rholang tests because the API of the Wallet and PoS will certainly change
migrate NonNegativeNumberTest.rho to RhoSpec
migrate EitherTest.rho to RhoSpec
implement Rholang logger `rho:io:stdlog` to help with contract debugging
migrate MakeMintTest.rho to RhoSpec.rho
add support for "== <-" assertions in RhoSpec
implement Rhospec.assertMany
use one assert method for all assertions
add support for test setup in RhoSpec.rho
Replace TestSet.rho with RhoSpec.rho
change asserts to receive an ack channel
add clues in the assertions so we can differentiate between different failures
fix the names and URNs of the assert primitives

@odeac odeac force-pushed the odeac:rhol-2292-part3 branch from e6ae9a8 to e194a37 Feb 4, 2019

@ArturGajowy

This comment has been minimized.

Copy link
Collaborator

ArturGajowy commented Feb 4, 2019

bors r+

bors bot added a commit that referenced this pull request Feb 4, 2019

Merge #2100 #2165
2100: implement RhoSpec contract in Rholang r=ArturGajowy a=odeac



2165: Clean up README.md r=adaszko a=adaszko

README.md is the first point of contact with the project.  It is our landing page.  It is very important it remains clean and lucid.

Co-authored-by: Ovidiu Deac <ovidiu.deac@rchain.coop>
Co-authored-by: Adam Szkoda <adaszko@gmail.com>
@bors

This comment has been minimized.

Copy link
Contributor

bors bot commented Feb 4, 2019

@bors bors bot merged commit e194a37 into rchain:dev Feb 4, 2019

2 checks passed

bors Build succeeded
Details
continuous-integration/drone/pr the build was successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment