trailofbits / necessist Goto Github PK

If I understand correctly, necessist will always mutate tests. However, we could also mutate the original source code and re-run the original tests to check if they pass. In that case, there is an issue with the tests.

Here is an example output on a Rust code base.

engine/src/multisig/client/keygen/tests.rs:72:2-72:28: `ceremony.complete().await;` passed
engine/src/multisig/client/keygen/tests.rs:105:2-105:28: `ceremony.complete().await;` passed

If statements become more complex and contain larger chains, it can become unclear which part has actually been removed. This could be shown in another color in the terminal output for example.

We may be able to use a uniform interface like https://github.com/langston-barrett/tree-sitter-edit to add support for additional languages and make others uniform if desired

https://github.com/trailofbits/necessist/blob/master/necessist/tests/sqlite_urls.rs
https://github.com/trailofbits/necessist/blob/master/necessist/tests/third_party.rs

At a minimum, this will require the third_party tests to support ssh urls and sqlite3 output.

The warning should give an example of (whichever applies):

• an unexpected database entry
• an entry expected to be in the database, but that is not

This would help with debugging, if nothing else.

This happens a lot with Go programs.

Example:

			t.Errorf("WriteT.%v: have err == nil, want non-nil", tv.Field(i).Type())
                        //                                                              ^^^^^^^

t.Errorf is ignored, but Necessist still tries to remove .Type().

This change probably should not be made until after the backends have been consolidated in the spirit of #247.

Necessist will warn that a test is passing with code remove for the following examples:

• A closure that contains an assertion

fn test1() {
std::thread::.spawn(move || {
                       ...
                        assert!(
                          ...
                        );
}

• A function call that contains an assertion

fn test2() {
   let x = ...
   let y = ...
    my_assertion_helper(x, y);
}
fn my_assertion_helper(x, y) { assert!(...); }

Both of these mutations do not indicate that a test statement is unnecessary, and I think they should be ignored since assert is ignored.

...to allow both configs and full set to true.

The install_node_modules function does not check if the package.json file exists in the project directory. This function is used for foundry projects as well to support foundry projects with hardhat dependencies. But dry run fails for the pure foundry projects because they don't have a package.json file.

Adding a check if the package.json file exists will resolve this issue.

Essentially, support this:

necessist <flags-for-necessist> -- <flags-to-pass-to-cargo-test>

E.g., whether these are current:

A similar test: https://github.com/trailofbits/test-fuzz/blob/ba63873e1fa2c7e44ad35039685c87975831c9c1/cargo-test-fuzz/tests/third_party.rs#L284-L306

Except we should probably use the latest release, similar to this: https://github.com/rust-fuzz/afl.rs/blob/4674dfeeb6445e97e23be25b47ae26968ec7a17a/.github/workflows/update_aflplusplus.yml#L22-L24

This could be realized almost immediately by embedding the script in the binary, writing the script out to a file, and executing it.

https://doc.rust-lang.org/std/macro.include_str.html

I see a few examples of where the exception list could be refined, however some might require some static analysis.
We could include exceptions for .function in vm.function and abi.function. With further analysis .Struct for Interface.Struct shouldn't be removed as well as .function in contract.function.

test/Test.t.sol:4457:11-4457:45: `.assume(_relayer > address(0x09));` nonbuildable
test/Test.t.sol:4458:9-4458:40: `vm.assume(_oracle != _relayer);` passed
test/Test.t.sol:4458:11-4458:40: `.assume(_oracle != _relayer);` nonbuildable
test/Test.t.sol:4461:9-4461:32: `vm.roll(startingBlock);` failed
test/Test.t.sol:4461:11-4461:32: `.roll(startingBlock);` nonbuildable
test/Test.t.sol:4466:35-4466:105: `.ProtocolFeeSettings(false, payable(address(0)), true, address(0), 0);` nonbuildable
test/Test.t.sol:4467:13-4467:91: `endpoint.updateProtocolFeeSettings("TestLibrary", 1, abi.encode(feeSettings));` passed
test/Test.t.sol:4467:21-4467:91: `.updateProtocolFeeSettings("TestLibrary", 1, abi.encode(feeSettings));` nonbuildable
test/Test.t.sol:4467:69-4467:89: `.encode(feeSettings)` nonbuildable
test/Test.t.sol:4470:46-4470:98: `.encode(false, address(_oracle), address(_relayer));` nonbuildable
test/Test.t.sol:4472:20-4472:121: `.encode(10, 10, 10, 10, _oracle, _relayer, address(this), address(0), address(0), true, true, false);` nonbuildable
test/Test.t.sol:4482:38-4482:49: `.encode(i);` nonbuildable
test/Test.t.sol:4483:45-4483:56: `.encode(i);` nonbuildable
test/Test.t.sol:4484:54-4484:71: `.encode(_relayer)` nonbuildable

E.g., in the following places:

Something that I think is happening now is:

• A dry-run fails.
• The associated tests are skipped.
• When later asked to resume, Necessist complains because of missing database entries.

In this issue we list the improvements that can be made to the foundry support:

1. Do not remove .expectCall lines in mutations
2. Do not remove .log lines in mutations

Add contribution guidelines with the following:

1. Describe different test suits and how to run them
2. General and CI tests that are required to be run to ensure correct formatting, linting, and other things
3. Documentation requirements if any

E.g.

Currently Necessist dry runs each test file and then starts mutating it line by line to find surviving mutations. The dry run of the next file required an additional build after reverting the last mutation of the earlier file. We can try to improve performance by dry running all the files together at the start, which will allow us to skip the build of the original code again and again with a dry run of every test file.

However, there is a challenge with this approach. The Necessist is built with two goals:

1. Provide useful information when something fails.
2. Be able to keep going when something fails.

Therefore to be able to find which test file had a failing test case will require parsing logic for test output for all supported frameworks. We are keeping this issue for future if we can find a better approach to handle this case.

However, for the long term, I am wondering if we can remove the REQUIRE_NODE_MODULES and simply rely on the presence of the package.json to decide if we should install node packages or not. There should be only rare cases in which the package.json exists and is not required to build or test the project.

Originally posted by @tarunbhm in #580 (comment)

What I imagine is that somehow, the arguments to this function are reworked, and a check is added involving the dump flag:

Here is where sqlite::init is called (you can see the dump flag is already incorporated into the must_not_exist argument):

Ideally, the fix would also include a trycmd test to verify the new behavior. That would involve adding a test to this directory: https://github.com/trailofbits/necessist/tree/master/core/tests/necessist_db_absent

Currently, --framework can be used to resolve ambiguities when multiple frameworks apply.

But Necessist will still error if it believes the specified framework does not apply.

Necessist should consider --framework a command, and should error only if it cannot be honored.