Pretty print expected json output of command line tests.

[ekpyron]

Came up in #7589 (comment).
Is there a nice and easy way to have a pretty printing pass on the output? That would make those tests way easier to review.

[leonardoalt]

The commandlineTests could jq the output, but not sure we want to add that dependency.

[cameel]

This should be very easy to do without extra dependencies now that #11583 is implemented. Just add the --pretty-print flag in cmdlineTests.sh, run it with --update and commit the result. I'm marking it as good first issue.

[0xGeorgii]

@matheusaaguiar May I take it?

[matheusaaguiar]

@GeorgePlotnikov yes, please! Let me know if you need any help.

[0xGeorgii]

@matheusaaguiar let me clarify I'm moving in the right direction. There is a script cmdlineTests.sh that runs cmdlineTests. The script executes test with the command_args="--standard-json "$(cat "${tdir}/args" 2>/dev/null || true).

The --standard-json arg which means I specify a test output in a json format, like here: linking_standard_solidity/output.json:

{"contracts":{"A":{"C":{"evm":{"bytecode":{"linkReferences":{},"object":"<BYTECODE REMOVED>"}}}}},"sources":{"A":{"id":0}}}

but we want to give a user opportunity to write an output in a --pretty-json format like that:

{
    "contracts":{
        "A":{
            "C":{
                "evm":{
                    "bytecode":{
                        "linkReferences":{
                            
                        },
                        "object":"<BYTECODE REMOVED>"
                    }
                }
            }
        }
    },
    "sources":{
        "A":{
            "id":0
        }
    }
}

My questions are:

1. Am I right that it is desirable to add a cmd line arg for the whole script like cmdlineTests.sh --pretty-json
2. If yes how the existing test cases should be managed:
   a. if there is no --pretty-json in the args or the output is not in a json format then it should be ignored?
   b. if the output not in the prettified json format then prettify it first, and compare afterwards?

[matheusaaguiar]

Yes, you are going in the right direction. I don't think we want to give the user the option of pretty-print, we actually want the json output of the relevant tests to be formatted in an easier way for humans to read. From a quick glance I think you can just add the --pretty-print flag to the command_args var and then run the script with the option --update. This in turn will automatically update the output of all tests and then it should be done.

[cameel]

Also, we then want to remove --pretty-print from args file in any test that already has it. We've been adding it only to get the formatting which will be automatic now.

Another thing, I'd recommend --pretty-print --json-indent 4 for more readable output, matching the length we have set in our .editorconfig.

Finally, we actually have some command-line tests that test the --pretty-print functionality itself. E.g. pretty_json_indent_only. We need a way to exempt them from this. My proposal would be to detect if the test dir contains a file called no-pretty-print and disable automatic pretty printing if it does.

[0xGeorgii]

hi @matheusaaguiar @cameel before proceeding with the main subject of this issue I decided to build and run all tests including both z3 and CVC4. I founded a small suspicious place but I'm not sure whether it is a known bit. If it is I can create a PR to fix it. If not please advise.

Here we have the following:

solidity/libsmtutil/CVC4Interface.cpp

Line 242 in b12b845

m_context.mkConst(CVC4::BitVector(1, size_t(0)))

but it the CVC4 bitvector.h there are two constructors that make this call ambiguous:

  BitVector(unsigned size, uint32_t z) : d_size(size), d_value(z)
  {
    d_value = d_value.modByPow2(size);
  }

  BitVector(unsigned size, uint64_t z) : d_size(size), d_value(z)
  {
    d_value = d_value.modByPow2(size);
  }

I suggest to put an explicit cast for a proper call. I have already test it on my environment, works fine.

[cameel]

I'm not sure it really matters in this case since this seems to be used to simply represent zero. Either type should work. Does it generate a warning?

@leonardoalt Want to take a look?

[0xGeorgii]

@cameel it even generates an error due to an ambiguous call

[cameel]

Oh. Right, we definitely want to fix that. Feel free to create a PR.

I wonder why our CI does not detect this. We surely have some runs with CVC4 enabled... What platform are you on?

[0xGeorgii]

@cameel I'm on MacOS 12.5
Apple clang version 14.0.0 (clang-1400.0.29.102)
Target: x86_64-apple-darwin21.6.0
Thread model: posix

[cameel]

Thanks. I guess that's it. We don't install CVC4 on macOS. Maybe we should. What do you think @leonardoalt?

[0xGeorgii]

Thanks. I guess that's it. We don't install CVC4 on macOS. Maybe we should. What do you think @leonardoalt?

PR #13556 created

[0xGeorgii]

@cameel @matheusaaguiar sorry for being a nerd but why there are byte/op codes, a source map sometimes are present in tests like here:

solidity/test/cmdlineTests/standard_yul_immutable_references/output.json

Line 15 in 3ddf5db

"object": "60298060156000396001600060010152806000f3fe7f000000000000000000000000000000000000000000000000000000000000000060005260206000f3",

but sometimes are not like here:

solidity/test/cmdlineTests/standard_yul_object/output.json

Line 17 in 3ddf5db

","bytecode":{"functionDebugData":{},"generatedSources":[],"linkReferences":{},"object":"<BYTECODE REMOVED>","opcodes":"<OPCODES REMOVED>","sourceMap":"<SOURCEMAP REMOVED>"}},"ir":"object \"NamedObject\" {

what should I do? substitute byte code (etc.) with the <BYTECODE REMOVED> or not?

[cameel]

It looks like the regexes in cmdlineTest.sh are just not versatile enough to strip those from pretty-printed JSON input. We strip bytecode from output because it contains metadata, which changes between versions and would make CI fail. So you need to update these regexes.

[0xGeorgii]

@cameel @matheusaaguiar PR #13586 was created please take a look

[0xGeorgii]

@cameel now it can be closed I guess

[0xGeorgii]

cool, thanks @matheusaaguiar for helping