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

Updated README.md file #12

Merged
merged 3 commits into from
Dec 7, 2023
Merged

Updated README.md file #12

merged 3 commits into from
Dec 7, 2023

Conversation

jferas
Copy link
Contributor

@jferas jferas commented Nov 14, 2023

No description provided.

Copy link

@alexkeating alexkeating left a comment

Choose a reason for hiding this comment

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

LGTM!

README.md Outdated

This `slither-args` field is where you can change the Slither configuration for your project, and the defaults above can of course be changed.
Some tests will not show up when running `scopelint spec` because the methods they are testing are inherited in the `RadworksGovernor`. In order to get an accurate picture of the tests with `scopelint spec` add an explicit `propose` method to the `RadworksGovernor`. It should look like this:
Copy link
Contributor

Choose a reason for hiding this comment

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

Currently this is the extent of what i see from scopelint spec. Do you see the same thing?
Screenshot 2023-11-23 at 10 52 33 PM

Choose a reason for hiding this comment

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

I see the same thing. I think this comment is saying if you want to see the tests on the propose method then you would have to add a propose method to the RadworksGovernor contract because scopelint does not show tests on inherited methods.

Copy link
Contributor

Choose a reason for hiding this comment

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

cool -- shouldn't it still show more tests though, the ones unrelated to propose? Anyway, don't let this block merge, if it's an issue in the future then we can revise.

Choose a reason for hiding this comment

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

At this point it the codebase it doesn't look like it should. It finds the tests to show based on the name of the test contracts. Right now the only test contracts with tests are Propose and Constructor

README.md Outdated
[^sarif]:
[SARIF](https://sarifweb.azurewebsites.net/) (Static Analysis Results Interchange Format) is an industry standard for static analysis results.
You can read learn more about SARIF [here](https://github.com/microsoft/sarif-tutorials) and read about GitHub's SARIF support [here](https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning).
This repo heavily leverages fuzz fork tests causing a significant number of RPC requests to be made. We leverage caching to minimize the number of RPC calls after the tests are run for the first time, but running these tests for the first may cause timeouts and consume a significant number of RPC calls.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
This repo heavily leverages fuzz fork tests causing a significant number of RPC requests to be made. We leverage caching to minimize the number of RPC calls after the tests are run for the first time, but running these tests for the first may cause timeouts and consume a significant number of RPC calls.
This repo heavily leverages fuzz fork tests causing a significant number of RPC requests to be made. We leverage caching to minimize the number of RPC calls after the tests are run for the first time, but running these tests for the first time may cause timeouts and consume a significant number of RPC calls.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done: 212a699

Copy link
Contributor

@wildmolasses wildmolasses left a comment

Choose a reason for hiding this comment

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

lgtm, mergeable

@jferas jferas changed the base branch from test-group-1 to main December 7, 2023 16:18
Copy link

github-actions bot commented Dec 7, 2023

Coverage after merging update-readme into test-group-1 will be

76.00%

Coverage Report
FileStmtsBranchesFuncsLinesUncovered Lines
src
   RadworksGovernor.sol73.91%75%69.23%76%101, 126–127, 135, 138, 152, 203, 84

@jferas jferas merged commit 3613326 into main Dec 7, 2023
9 of 11 checks passed
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.

3 participants