slither icon indicating copy to clipboard operation
slither copied to clipboard

Test for unification of path filtering across POSIX and Windows

Open pcaversaccio opened this issue 3 years ago • 5 comments

As discussed with @0xalpharush in https://github.com/crytic/slither/pull/1196 I now wrote a small test suite that checks if path filtering across POSIX and Windows works.

The CI tests will fail with the error "Path filtering across POSIX and Windows failed" because you use the master branch in the CI to set up Slither. So this is a good error :-D. I tested it here.

The strategy I took here is that I made a couple of contracts that contain reentrancy issues as dependencies to a main (empty) contract. Now if you filter out the paths properly, the output should be zero result(s) found.

I hope the code is in line with your internal rules (not sure about the solc version for the test contracts).

pcaversaccio avatar Jul 26 '22 16:07 pcaversaccio

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Jul 26 '22 16:07 CLAassistant

@montyly I just saw you re-triggered the actions. Is there something wrong with the code?

As mentioned above, the reason why it's failing (and it's actually good) is because of how you set up the CI flow.

You use python setup.py install and in the setup.py you use Slither version 0.8.3 which does not contain the fix from https://github.com/crytic/slither/pull/1196.

pcaversaccio avatar Jul 27 '22 09:07 pcaversaccio

@pcaversaccio: my bad; there were some timeout issues in the CI due to too many urls requests, so I just went ahead an re-run the recent PRs with failing actions to see if they were affected :)

montyly avatar Jul 27 '22 10:07 montyly

@montyly lol - had a similar problem recently.

pcaversaccio avatar Jul 27 '22 10:07 pcaversaccio

@0xalpharush @montyly any feedback on this PR? IMHO it's ready to be merged.

pcaversaccio avatar Sep 05 '22 17:09 pcaversaccio

hey guys, @0xalpharush @montyly - it's been already some time since I prepared that PR. Would appreciate some feedback or let's get it merged if you agree, thx!

pcaversaccio avatar Sep 25 '22 10:09 pcaversaccio

Thanks for the PR @pcaversaccio & apologies for the delayed review! I've left some comments inline, but overall this looks ok. The only thing that needs to be addressed is the change due to the introduction of the new --fail-* options in slither. If you could merge dev into your branch to get the new code in and address the comments in the PR that'd be awesome!

elopez avatar Sep 25 '22 23:09 elopez

@elopez thanks for the feedback - dev is merged into my branch via https://github.com/pcaversaccio/slither/pull/1. I searched for your inline comments but couldn't find anything - could you please point me to your feedback, thx.

pcaversaccio avatar Sep 26 '22 07:09 pcaversaccio

It looks like the CI test fails @pcaversaccio 🤔 could you take a look? I suspect it might need solc-select install & use 0.8.0 in the script (the pipeline runs with a 0.5.x version otherwise)

elopez avatar Sep 26 '22 13:09 elopez

@elopez I already indicated here why the tests are failing. Since we test against Slither version 0.8.3 which does not contain your fix the test is failing (which is actually good since it does its job). Don't know how to resolve this tbh.

image

pcaversaccio avatar Sep 26 '22 14:09 pcaversaccio

@pcaversaccio the tests install slither from dev (or whatever the PR branch is built off of) source code, which already has the changes in #1196 merged in, so the tests should be passing. The version number is not relevant for this, as the release is not pulled from PyPI. I think it's just a matter of the test solidity code failing to build, as the CI pipeline uses solc 0.5.x and the code requires 0.8.x

elopez avatar Sep 26 '22 14:09 elopez

@elopez you are completely right! I pushed a fix - could re-trigger the actions?

pcaversaccio avatar Sep 26 '22 14:09 pcaversaccio

yeah :-D everything green. thx for the hint!

pcaversaccio avatar Sep 26 '22 14:09 pcaversaccio

This is great, thanks @pcaversaccio

montyly avatar Oct 03 '22 08:10 montyly