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

Pre-release warning stripping in CLI tests does not always produce correct JSON formatting when combined with --pretty-print #13544

Closed
cameel opened this issue Sep 20, 2022 · 0 comments · Fixed by #13549
Labels
bug 🐛 good first issue 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. should have We like the idea but it’s not important enough to be a part of the roadmap. testing 🔨

Comments

@cameel
Copy link
Member

cameel commented Sep 20, 2022

This came up in #13162 (review).

Non-release builds of the compiler always output an extra warning, which results in a different output between release and debug builds in CI:

Warning: This is a pre-release compiler version, please do not use it in production.

For this reason cmdlineTests.sh script strips the warning from compile output. For Standard JSON this is more tricky, since the error is a part of an array and we have to match the JSON formatting of the expectations. The stripping code seems to have a small bug because in some cases the formatting it produces after stripping the warning differs from what we get when the warning is not there at all.

We need to adjust the stripping code to handle this corner case. It would be best if we could also solve the problem more generally by making the CLI tests simply ignore the JSON formatting.

Fixing this is likely a prerequisite for #7665, since enabling pretty printing may uncover this problem in other existing tests.

How to reproduce

Let's look at the prettified Standard JSON output for this input.json file:

{
    "language": "Solidity",
    "sources": {
        "url_not_found.sol": {
            "urls": ["contract.sol"]
        }
    }
}
solc --standard-json input.json --pretty-json --json-indent 4

This is what the output of a pre-release binary looks like unstripped:

{
    "errors":
    [
        {
            "component": "general",
            "formattedMessage": "Cannot import url (\"contract.sol\"): File not found. Searched the following locations: \"\".",
            "message": "Cannot import url (\"contract.sol\"): File not found. Searched the following locations: \"\".",
            "severity": "error",
            "type": "IOError"
        },
        {
            "component": "general",
            "errorCode": "3805",
            "formattedMessage": "Warning: This is a pre-release compiler version, please do not use it in production.\n\n",
            "message": "This is a pre-release compiler version, please do not use it in production.",
            "severity": "warning",
            "type": "Warning"
        }
    ],
    "sources": {}
}

The stripping in cmdlineTests.sh removes the pre-release warning, which results in this output:

{
    "errors":
    [
        {
            "component": "general",
            "formattedMessage": "Cannot import url (\"contract.sol\"): File not found. Searched the following locations: \"\".",
            "message": "Cannot import url (\"contract.sol\"): File not found. Searched the following locations: \"\".",
            "severity": "error",
            "type": "IOError"
        }],
    "sources": {}
}

Note the }], sequence in the output. A release binary, that does not output the warning produces a slightly different formatting in that spot:

{
    "errors":
    [
        {
            "component": "general",
            "formattedMessage": "Cannot import url (\"contract.sol\"): File not found. Searched the following locations: \"\".",
            "message": "Cannot import url (\"contract.sol\"): File not found. Searched the following locations: \"\".",
            "severity": "error",
            "type": "IOError"
        }
    ],
    "sources": {}
}

As a result, it's impossible to write a test with nicely formatted JSON output that matches both release and pre-release build output. For example in #13162 t_ubu_release fails if we expect }], and t_ubu_cli fails if we expect the other variant.

The warning stripping code

You can find the code responsible for stripping this warning here:

if [[ " ${solc_args[*]} " == *" --standard-json "* ]]
then
python3 - <<EOF
import re, sys
json = open("$stdout_path", "r").read()
json = re.sub(r"{[^{}]*Warning: This is a pre-release compiler version[^{}]*},?", "", json)
json = re.sub(r"},\s*]", "}]", json) # },] -> }]
json = re.sub(r"\"errors\":\s*\[\s*\],?\s*","",json) # Remove "errors" array if it's not empty
json = re.sub("\n\\s+\n", "\n\n", json) # Remove any leftover trailing whitespace
open("$stdout_path", "w").write(json)
EOF

@cameel cameel added bug 🐛 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. should have We like the idea but it’s not important enough to be a part of the roadmap. labels Sep 20, 2022
@cameel cameel changed the title Pre-release warning stripping in CLI tests does not always produce correct indentation when combined with --pretty-print Pre-release warning stripping in CLI tests does not always produce correct JSON formatting when combined with --pretty-print Sep 20, 2022
@cameel cameel moved this to Triage in Solidity Focus Board Sep 20, 2022
@cameel cameel moved this from Triage to Next 10 actionable tasks in Solidity Focus Board Sep 20, 2022
@cameel cameel moved this from Next 10 actionable tasks to In progress in Solidity Focus Board Sep 21, 2022
Repository owner moved this from In progress to Done in Solidity Focus Board Sep 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 good first issue 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. should have We like the idea but it’s not important enough to be a part of the roadmap. testing 🔨
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant