protostar icon indicating copy to clipboard operation
protostar copied to clipboard

Support code coverage

Open kasperski95 opened this issue 2 years ago • 9 comments

https://github.com/starknet-edu/cairo-coverage

kasperski95 avatar Aug 01 '22 19:08 kasperski95

One of our interns at Nethermind @machard is interested in adding this to protostar. He's already got a minimal MVP by instrumenting function entry points with hints: image @kasperski95 mind if he takes it on?

JorikSchellekens avatar Aug 23 '22 18:08 JorikSchellekens

@JorikSchellekens Yes, he can take this task

kasperski95 avatar Aug 24 '22 08:08 kasperski95

@machard Hey, can you tell us in a few points how do you plan to implement this feature? I'm asking because, we'd like to minimize merge conflicts.

cc @mkaput

kasperski95 avatar Aug 24 '22 08:08 kasperski95

@machard Not only to minimize merge conflicts, we would just like to be aligned with the ongoing work in test runner 😄 Its internals are not documented anywhere, and we have our own in-house intuition and plans for this.

For example, I'm currently working on an ability to configure test cases in Cairo code from a new kind of hook functions called setup cases (for test_my_test_name the setup case would be called setup_my_test_name). I guess you might be interested in adding an ability to configure coverage test-by-test in setup cases (doesn't have to happen, though! 🙂). My work includes a sizeable refactoring of test case runner internals, and it might be worth it to synchronize both pieces of work.

mkaput avatar Aug 24 '22 08:08 mkaput

Hi,

So far it's a standalone PoC that you can find here https://github.com/machard/protostar-coverage-poc

  • it parses the Cairo file to AST
  • count some stuff in the AST
  • instrument the AST and expose everything through context
  • write a new instrumented file
  • there is a cheat code currently : the test is responsible to display the coverage result available in context

I haven't looked into protostar codebase yet. But I was thinking:

  • Protostar probably parse cairo file to AST ? In this case I was thinking to instrument on the fly here
  • Protostar has access to context, so I was thinking to display coverage info at the same time protostar displays "Tests suite: x/x..."

I think the conflicting areas would be small. However, I do see potentially annoying stuff to reconcile lines numbers when an error happens in a test ⚠️ . Not sure exactly how to handle that yet.

However, there's also the possibility to continue to work on it as standalone tool for now.

It's still very much early stage. For example I haven't tried the instrumentation on various codebases and use cases. Also it currently just counts functions and statements executed.

So I guess what you can tell me:

  • Do I keep it as a standalone thing for now, or do I do a PoC integration into protostar to check if my idea of integration is ok ?
  • Should I focus for now on having the standalone PoC running on any kind of codebase/usecase ? in that case maybe you have a list of codebases to try it out ?
  • Should I try to see if some other feature is doable ?
  • Any other thing you want 😃

machard avatar Aug 24 '22 12:08 machard

  • Protostar probably parse cairo file to AST ? In this case I was thinking to instrument on the fly here

Yes, we do even modify the Cairo compiler with our own compilation passes. You should be easily able to inject your Instrument visitor there.

https://github.com/software-mansion/protostar/blob/6048e1ba3553aa264f9a8c9e9b923a4651484ab0/protostar/utils/compiler/pass_managers.py

  • Protostar has access to context, so I was thinking to display coverage info at the same time protostar displays "Tests suite: x/x..."

We have an object called ExecutionState and test-specific subclass called TestExecutionState. Your context can easily be put there as a field and then used from cheatcodes and thus -- available to instrumentation hints.

I do a very similar thing in the yet in progress max_examples cheatcode. You can peek the WIP commit here: https://github.com/software-mansion/protostar/commit/5510f43c33b28fc8a700b4d531795ac992aff58f

However, I do see potentially annoying stuff to reconcile lines numbers when an error happens in a test ⚠️ . Not sure exactly how to handle that yet.

Just do not manipulate the location field in all AST structures, and use the location of CodeElementFunction as the location of your injected hints. That's what cairo-lang does itself, for example in the if a and b syntax 😉

I think the conflicting areas would be small.

With your approach, it looks like so! FYI The results of coverage can be put in the ExecutionResourcesSummary class, instead of printing these to stdout.


In addition, you might research hooking into our subclass of Cairo VM which we use in tests: CheatableVirtualMachine. At this point, you have access to any Cairo instruction that is executed in the program. I don't know how much about the program structure you can reconstruct though in this scenario, because you are now effectively observing CASM (Cairo Assembly).


Nonetheless, I wonder how much of this is already done by Cairo tracer. @MaksymilianDemitraszek is currently very slowly working on a Cairo profiler which uses the mentioned utility. He wrote down his observations here: https://github.com/software-mansion/protostar/discussions/542. I would like to hear from Maks his opinion about this.

In general, if Maks doesn't see anything against, you have a green light from me to work on this directly in Protostar. Let's hear the voices from the rest of the team, though. We need to think more how to organize the work.

mkaput avatar Aug 24 '22 14:08 mkaput

@machard Thanks for taking interest in contribution to our project! I am more than excited for this feature. ❤️

As @mkaput said I am currently working on the cairo profiler. I believe using hints for your problem might be, great and very creative, but not the best approach - in terms of maintainability and simplicity.

What I would suggest is using cairo trace and debug info which would provide you the mapping from assembly to instructions/functions lines and let you deduce covered code.

Files which may be interesing to you:

  • https://github.com/starkware-libs/cairo-lang/blob/master/src/starkware/cairo/lang/tracer/tracer_data.py
  • https://github.com/starkware-libs/cairo-lang/blob/master/src/starkware/cairo/lang/tracer/profiler.py

I would be more than happy to assist or help you with any matter regarding this feature or cairo trace/cairo vm.

If you need any help feel free to write to me on Starknet discord.

MaksymilianDemitraszek avatar Aug 25 '22 14:08 MaksymilianDemitraszek

for posterity, as i'll focus on learning cairo, i'm leaving that here : https://github.com/software-mansion/protostar/compare/master...machard:coverage-poc?expand=1 in short, with the current codebase, the printed lines seems to contains correct locations for test files and files imported directly from the test files. However lines referring to files deployed with deploy_contract will be referring to the "autogen" code, which is not what we want.

machard avatar Sep 05 '22 16:09 machard

We will investigate integrating https://github.com/starknet-edu/cairo-coverage into Protostar

mkaput avatar Nov 09 '22 08:11 mkaput