Set handlers use unified jsonPrint with prettifier if needed

[akali]

Partially addresses #12011 (all except for --metadata).

[cameel]

Thanks for the PR! This looks good overall but tests in CI are failing. You need to update tests so that the output matches expectations.

• in t_ubu_cli the test named ast_compact_json_with_base_path is failing because --ast-compact-json used to pretty-print the output by default. Now it no longer does. You need to add --pretty-print to its args file to perserve the current output. There might be more tests that need a similar change. You can run the CLI tests locally with commandLineTests.sh
• Lots of test cases fail in t_ubu_soltest. Same reason as above. To fix them you need to modify the test case runners to pass the right options to the functions that generate JSON. For example this needs to be done in ABIJsonTest: https://github.com/ethereum/solidity/blob/ef21e43fa36be8ff58725133aa5bb6e74bd06baf/test/libsolidity/ABIJsonTest.cpp#L64
  This is of course not the only place.
• Please add new CLI tests that cover both pretty-printed and normal output for these options so that we can see that the change works. Take a look at #11639 if you want to see how to add such tests.
• Finally, since this does not cover --metadata, please remove Closes #12011 from PR description. We don't want to close the issue before all options are addressed. But doing the metadata separately is definitely a good idea since this one might require some discussion in the team while the other options should not be controversial at all.

[cameel]

Do you need any help with adding/updating these tests?

[dallonasnes]

I'd be happy to update the tests if that's still blocking merge

[cameel]

Sure. Looks like @akali is not working on it so it's fine to take over.

[dallonasnes]

Is there a way to disable tests that require SMT/Horn solver in test/cmdLineTests.sh?

[cameel]

It supports the --no-smt option just like soltest/isoltest.
You can also give it a pattern with shell wildcards to run only the tests with matching names.

[dallonasnes]

How do you input the pattern? Here's what I'm trying to run, but it doesn't like the arg format

./cmdlineTests.sh ast_compact_json_with_base_path

[cameel]

That should work if it's the full name of the test dir. If not, you have to add some wildcards:

./cmdlineTests.sh "*ast_compact_json_with_base_path*"

[dallonasnes]

test git:(fix-tests) $ ./cmdlineTests.sh "*ast_compact_json_with_base_path*"
cut: illegal option -- -
usage: cut -b list [-n] [file ...]
       cut -c list [file ...]
       cut -f list [-s] [-d delim] [file ...]

[cameel]

Looks like you're having a problem with this line:
https://github.com/ethereum/solidity/blob/e0c85c6f58310e61dbc6f153b6228e6240e63f8a/test/cmdlineTests.sh#L57

Are you by chance on macOS? It's probably that cut on your platform supports only short options. Replacing cut --characters with cut -c should fix that. If that works for you, please submit a PR with the change.

This probably slipped though because we never really use this feature in CI - we always run all tests - and locally it worked for me on Linux so I never noticed.

[dallonasnes]

Yup I'm on macOS. I'll try that out, thanks

[dallonasnes]

The -c fix worked for me. PR #12348

[cameel]

@dallonasnes Are you still interested in working on this?

[dallonasnes]

Hi, better if someone else takes it. For tests to pass, it'll be small changes in many dozens of files, hadn't realized it was that broad of a change.

[cameel]

@dallonasnes Test input must be updated, yes, but if you mean test output/expectations, we of course have auto-update mode so it does not have to be done manually. I.e. there's the --accept-updates option for isoltest and --update for cmdlineTests.sh.

[wechman]

I take over this PR.

[wechman]

@dallonasnes Test input must be updated, yes, but if you mean test output/expectations, we of course have auto-update mode so it does not have to be done manually. I.e. there's the --accept-updates option for isoltest and --update for cmdlineTests.sh.

@cameel, I guess this is not the best way to go. We want to keep pretty JSONs in the test expectations and modify only the test runners, right? Please check a fixup commit I delivered and let me know it is fine.

[cameel]

@wechman Ah, you're right! Looks like we were already pretty-printing by default in all cases so we can actually keep the expectations and just adjust the options. I misremembered that and thought that we needed to update expectation in some cases too but apparently not.

This still needs some tests for the compact version though.

[wechman]

@cameel, I added compact JSON output tests to the fixup commit. But, I noticed surprising result in ast_pretty_json_with_base_path test. There is pretty-printed JSON with following first line: "JSON AST (compact format):" in the test output. I am not sure if I the information about "compact format" is required to be part of the output, is it? Unless, I should not mix "--ast-compact-json" with "--pretty-json" in the test scenarios?

[cameel]

The compact part in --ast-compact-json is not related to pretty-printing. There used to be an option called --ast-json and it was removed in #10363. The new format is just a bit more dense. It's unfortunate that it's now easily confused with compact printing.

Maybe we could remove the "compact format" part from the header, though changing the output is technically a breaking change. Personally, I'd just leave it as is, it does correspond to the option name.

Anyway, the output of --ast-compact-json should be affected by --pretty-print just like the other options.

[cameel]

I added some suggestions about test naming.

The remaining issues are that --pretty-print is meant to affect --ast-compact-json (so that needs to be changed back) and that some of the options covered by the PR appear not to be fully covered.

[wechman]

@cameel Thank you all your comments! I updated code accordingly. I also squashed commits and rebased them on the newest develop.

[cameel]

Looks good.

I just noticed that it's missing a changelog entry though. This is a change visible to users.

[wechman]

@cameel Changelog updated.

[leonardoalt]

@wechman not sure if you just rebased it, but cli tests failing

[cameel]

Yeah, needs a rebase and updating CLI test expectations but other than that it's done.

[wechman]

@leonardoalt @cameel I have just rebased. Also, I have updated expectations in one command line test case.

[Sticknmove]

Thanks

[Sticknmove]

Thanks

[Sticknmove]

Thanks

[Sticknmove]

Thanks

[Sticknmove]

Thanks

[Sticknmove]

Thanks

[Sticknmove]

Thanks