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

Rewrite FunctionCallGraph tests as soltest test suite #13426

Closed
cameel opened this issue Aug 23, 2022 · 5 comments
Closed

Rewrite FunctionCallGraph tests as soltest test suite #13426

cameel opened this issue Aug 23, 2022 · 5 comments
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. good first issue candidate Could be a "good first issue" but something is blocking it or it has open questions. low impact Changes are not very noticeable or potential benefits are limited. medium effort Default level of effort nice to have We don’t see a good reason not to have it but won’t go out of our way to implement it. stale The issue/PR was marked as stale because it has been open for too long. testing 🔨

Comments

@cameel
Copy link
Member

cameel commented Aug 23, 2022

FunctionCallGraph is currently covered with boost tests. These tests are generally harder to maintain and update than our soltest-style tests.

We should define a custom test case class for them and put expectations in annotations. See StackLayoutGeneratorTest and test/libyul/yulStackLayout/ for an example of such a custom test suite and test cases based on it.

Example

We should be able to replace the following test case:

BOOST_AUTO_TEST_CASE(only_definitions)
{
unique_ptr<CompilerStack> compilerStack = parseAndAnalyzeContracts(R"(
function free() {}
library L {
function ext() external {}
function pub() public {}
function inr() internal {}
function prv() private {}
}
contract C {
function ext() external {}
function pub() public {}
function inr() internal {}
function prv() private {}
}
)"s);
tuple<CallGraphMap, CallGraphMap> graphs = collectGraphs(*compilerStack);
map<string, EdgeNames> expectedCreationEdges = {
{"C", {}},
{"L", {}},
};
map<string, EdgeNames> expectedDeployedEdges = {
{"C", {
{"Entry", "function C.ext()"},
{"Entry", "function C.pub()"},
}},
{"L", {
{"Entry", "function L.ext()"},
{"Entry", "function L.pub()"},
}},
};
checkCallGraphExpectations(get<0>(graphs), expectedCreationEdges);
checkCallGraphExpectations(get<1>(graphs), expectedDeployedEdges);
}

with a .sol file that looks like this:

function free() {}

library L {
    function ext() external {}
    function pub() public {}
    function inr() internal {}
    function prv() private {}
}

contract C {
    function ext() external {}
    function pub() public {}
    function inr() internal {}
    function prv() private {}
}
// ----
// CREATION
//     contract C
//     library L
// DEPLOYED
//     contract C
//         Entry -> function C.ext()
//         Entry -> function C.pub()
//     library L
//         Entry -> function L.ext()
//         Entry -> function L.pub()
@cameel cameel added good first issue testing 🔨 low effort There is not much implementation work to be done. The task is very easy or tiny. low impact Changes are not very noticeable or potential benefits are limited. nice to have We don’t see a good reason not to have it but won’t go out of our way to implement it. medium effort Default level of effort and removed low effort There is not much implementation work to be done. The task is very easy or tiny. labels Aug 23, 2022
@0xGeorgii
Copy link
Contributor

Hi @cameel I'm new to Solidity but want to dig deeper into it. May I elaborate on this issue and provide an improvement? It takes some time and I will ask questions. Hope it won't be too bothering.

@matheusaaguiar
Copy link
Collaborator

Hi @GeorgePlotnikov , maybe this issue is not really a good first issue for someone starting out, because it could demand understanding of how other test classes are implemented. I am not sure how long you are digging into solidity or how much time you are dedicating to it. So in order to give you more options, I'd like to suggest some other good first issues:

@0xGeorgii
Copy link
Contributor

hi @matheusaaguiar thanks for clarifying. Let me consider these two before. If this one will be still orphaned I grab it.

@NunoFilipeSantos NunoFilipeSantos added the good first issue candidate Could be a "good first issue" but something is blocking it or it has open questions. label Dec 5, 2022
@github-actions
Copy link

This issue has been marked as stale due to inactivity for the last 90 days.
It will be automatically closed in 7 days.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Mar 30, 2023
@github-actions
Copy link

github-actions bot commented Apr 7, 2023

Hi everyone! This issue has been automatically closed due to inactivity.
If you think this issue is still relevant in the latest Solidity version and you have something to contribute, feel free to reopen.
However, unless the issue is a concrete proposal that can be implemented, we recommend starting a language discussion on the forum instead.

@github-actions github-actions bot added the closed due inactivity The issue/PR was automatically closed due to inactivity. label Apr 7, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
closed due inactivity The issue/PR was automatically closed due to inactivity. good first issue candidate Could be a "good first issue" but something is blocking it or it has open questions. low impact Changes are not very noticeable or potential benefits are limited. medium effort Default level of effort nice to have We don’t see a good reason not to have it but won’t go out of our way to implement it. stale The issue/PR was marked as stale because it has been open for too long. testing 🔨
Projects
None yet
Development

No branches or pull requests

4 participants