scripts/ImportExportTest.sh: replace scripts/ASTImportTest.sh. #12809

aarlt · 2022-03-16T16:11:05Z

Replaces part of #12704.
Moved to #12834.

replaces scripts/ASTImportTest.sh with scripts/ImportExportTest.sh
scripts/ImportExportTest.sh is already prepared to test asm-json import/export, but right now these tests are just not triggered.

wechman

It would be nice to have at least one test with asm-json before a merge.

wechman · 2022-03-25T11:53:00Z

scripts/ImportExportTest.sh

+            fi
+            set +e
+            DIFF="$(diff expected.json obtained.json)"
+            set +e


Shouldn't it be set -e?

Oh that's true! That seem to be a typo! Thx!

wechman · 2022-03-25T12:01:22Z

scripts/ImportExportTest.sh

+if [[ "$OSTYPE" == "darwin"* ]]; then
+    READLINK=greadlink
+fi
+IMPORT_TEST_TYPE=${1}


Is ${1} somehow better than "$1"?

Nope, not really.. but you're right, this should be "${1}".

They're equivalent but ${} ensures that the text after the variable name won't be interpreted as a part of that name. Personally, I use the "$x" form only there's nothing else inside the quotes. If there's extra text, other variables, etc., I always use ${x}.

wechman · 2022-03-25T12:08:07Z

scripts/ImportExportTest.sh

+                                    jq --raw-output ".contracts.${obtained_contract}.\"asm\"" obtained_direct_import_export.json > obtained_direct_import_export.asm
+                                    set +e
+                                    DIFF="$(diff expected.asm obtained_direct_import_export.asm)"
+                                    set -e
+                                    if [ "$DIFF" != "" ]
+                                    then
+                                        if [ "$DIFFVIEW" == "" ]
+                                        then
+                                            echo -e "ERROR: JSONS differ for $1: \n $DIFF \n"
+                                            echo "Expected:"
+                                            cat  expected.asm
+                                            echo "Obtained:"
+                                            cat obtained_direct_import_export.asm
+                                        else
+                                            # Use user supplied diff view binary
+                                            $DIFFVIEW expected.asm obtained_direct_import_export.asm


Looks like duplication of lines 124-143. Could you take out this code to the helper function?

True! I will add some helpers.

wechman · 2022-03-25T12:29:18Z

scripts/ImportExportTest.sh

+# Expected parameters:
+# $1 name of the file to be exported and imported
+# $2 any files needed to do so that might be in parent directories
+function testImportExportEquivalence {


It would be great to have this function divided into smaller pieces since it is quite big by now.

wechman · 2022-03-25T12:32:07Z

scripts/ImportExportTest.sh

+    exit 1
+fi
+
+# function tests whether exporting and importing again leaves the JSON ast unchanged


I think the comment may require an update.

aarlt · 2022-03-28T16:39:39Z

Moved to #12834. Closing.

cameel

Oh, I was in the middle of reviewing this...
Looks like I'll need to move my comments to the new PR.

cameel · 2022-03-28T15:52:15Z

scripts/ASTImportTest.sh

@@ -1,155 +0,0 @@
-#!/usr/bin/env bash


Could you do the file rename in a separate commit? I think this would help git detect that it's still basically the same script and show a better diff.

cameel · 2022-03-28T15:53:55Z

scripts/ImportExportTest.sh

+            set +e
+            DIFF="$(diff expected.json obtained.json)"
+            set +e


Suggested change

set +e

DIFF="$(diff expected.json obtained.json)"

set +e

DIFF="$(diff expected.json obtained.json || true)"

cameel · 2022-03-28T16:12:17Z

scripts/ImportExportTest.sh

+if [[ "$OSTYPE" == "darwin"* ]]; then
+    READLINK=greadlink
+fi
+IMPORT_TEST_TYPE=${1}


They're equivalent but ${} ensures that the text after the variable name won't be interpreted as a part of that name. Personally, I use the "$x" form only there's nothing else inside the quotes. If there's extra text, other variables, etc., I always use ${x}.

cameel · 2022-03-28T16:13:09Z

scripts/ImportExportTest.sh

+    then
+        ! [[ -e stderr.txt ]] || { echo "stderr.txt already exists. Refusing to overwrite."; exit 1; }
+
+        if [ "${IMPORT_TEST_TYPE}" == "ast" ]


Let's use the modern Bash variant.

Suggested change

if [ "${IMPORT_TEST_TYPE}" == "ast" ]

if [[ $IMPORT_TEST_TYPE == "ast" ]]

Same in the elif below.

cameel · 2022-03-28T16:14:58Z

test/cmdlineTests.sh

 SOLTMPDIR=$(mktemp -d)
 (
    cd "$SOLTMPDIR"
-    if ! "$REPO_ROOT/scripts/ASTImportTest.sh"
+    if ! "$REPO_ROOT/scripts/ImportExportTest.sh" ast


Suggested change

if ! "$REPO_ROOT/scripts/ImportExportTest.sh" ast

if ! "$REPO_ROOT/scripts/importExportTest.sh" ast

cameel · 2022-03-28T16:23:29Z

scripts/ImportExportTest.sh

+if [[ "$OSTYPE" == "darwin"* ]]; then
+    READLINK=greadlink
+fi
+IMPORT_TEST_TYPE=${1}


I'd put commandline argument processing as the first thing in the script. Just after set -e.

cameel · 2022-03-28T16:25:43Z

scripts/ImportExportTest.sh

@@ -0,0 +1,302 @@
+#!/usr/bin/env bash
+
+set -e


Would this script work with these?

Suggested change

set -e

set -euo pipefail

I wanted to do that last time I was going through these scripts but I did not have time to test each one individually and make sure it does not break anything. Now that you're rewriting it, it looks like a good opportunity to try this.

cameel · 2022-03-28T16:27:35Z

scripts/ImportExportTest.sh

+# boost_filesystem_bug specifically tests a local fix for a boost::filesystem
+# bug. Since the test involves a malformed path, there is no point in running
+# AST tests on it. See https://github.com/boostorg/filesystem/issues/176
+if [ "${IMPORT_TEST_TYPE}" == "ast" ]


Suggested change

if [ "${IMPORT_TEST_TYPE}" == "ast" ]

if [[ $IMPORT_TEST_TYPE == "ast" ]]

cameel · 2022-03-28T16:28:39Z

scripts/ImportExportTest.sh

+    echo "unknown import test type. aborting. please specify $0 [ast|evm-assembly]."
+    exit 1


Suggested change

echo "unknown import test type. aborting. please specify $0 [ast|evm-assembly]."

exit 1

fail "Unknown import test type. Aborting. Please specify ${0} [ast|evm-assembly]."

cameel · 2022-03-28T16:33:12Z

scripts/ImportExportTest.sh

+# AST tests on it. See https://github.com/boostorg/filesystem/issues/176
+if [ "${IMPORT_TEST_TYPE}" == "ast" ]
+then
+    IMPORT_TEST_FILES=$(find "${SYNTAXTESTS_DIR}" "${ASTJSONTESTS_DIR}" -name "*.sol" -and -not -name "boost_filesystem_bug.sol")


To avoid repeating the whole command, you could just set the list of dirs here:

Suggested change

IMPORT_TEST_FILES=$(find "${SYNTAXTESTS_DIR}" "${ASTJSONTESTS_DIR}" -name "*.sol" -and -not -name "boost_filesystem_bug.sol")

TEST_DIRS=("$SYNTAXTESTS_DIR" "$ASTJSONTESTS_DIR")

Then have this for both cases:

IMPORT_TEST_FILES=$(find "${TEST_DIRS[@]}" -name "*.sol" -and -not -name "boost_filesystem_bug.sol")

aarlt · 2022-03-28T17:01:15Z

Oh, I was in the middle of reviewing this...
Looks like I'll need to move my comments to the new PR.

Oh!! Sorry for that! I think there is no need to move the comments.. I will try to integrate your feedback from here on the other PR.. I hope it will not create too much confusion

cameel · 2022-03-28T17:48:20Z

Well, you did say on the meeting that you'll be moving this to a bigger PR where tests actually test something so it's my fault. But no worries. Just copying them fortunately was not that bad. They're all in #12834 now (except 1 or 2 that were basically just responses to other comments).

aarlt force-pushed the import-export-script branch from 8ce386d to 083d79a Compare March 16, 2022 16:52

cameel added the testing 🔨 label Mar 18, 2022

aarlt changed the base branch from develop to asm-json-export March 21, 2022 16:39

aarlt marked this pull request as draft March 21, 2022 16:40

aarlt force-pushed the asm-json-export branch 5 times, most recently from e4acc25 to b9504ee Compare March 22, 2022 16:00

aarlt force-pushed the import-export-script branch from 083d79a to 6fe5e7e Compare March 22, 2022 22:03

This was referenced Mar 22, 2022

Add --import-asm-json input mode. #12704

Closed

libevmasm: Add support to import evm assembly json. #12834

Closed

aarlt force-pushed the asm-json-export branch 2 times, most recently from f3dd7cd to 1a0988e Compare March 23, 2022 20:58

aarlt force-pushed the import-export-script branch from 6fe5e7e to 24bebf5 Compare March 23, 2022 20:58

scripts/ImportExportTest.sh: replace scripts/ASTImportTest.sh.

ebac6c5

aarlt force-pushed the import-export-script branch from 24bebf5 to ebac6c5 Compare March 23, 2022 21:10

Base automatically changed from asm-json-export to develop March 24, 2022 11:13

wechman reviewed Mar 25, 2022

View reviewed changes

aarlt closed this Mar 28, 2022

cameel reviewed Mar 28, 2022

View reviewed changes

	if [ "${IMPORT_TEST_TYPE}" == "ast" ]
	if [[ $IMPORT_TEST_TYPE == "ast" ]]

	if ! "$REPO_ROOT/scripts/ImportExportTest.sh" ast
	if ! "$REPO_ROOT/scripts/importExportTest.sh" ast

		echo "unknown import test type. aborting. please specify $0 [ast\|evm-assembly]."
		exit 1

	echo "unknown import test type. aborting. please specify $0 [ast\|evm-assembly]."
	exit 1
	fail "Unknown import test type. Aborting. Please specify ${0} [ast\|evm-assembly]."

	IMPORT_TEST_FILES=$(find "${SYNTAXTESTS_DIR}" "${ASTJSONTESTS_DIR}" -name "*.sol" -and -not -name "boost_filesystem_bug.sol")
	TEST_DIRS=("$SYNTAXTESTS_DIR" "$ASTJSONTESTS_DIR")

scripts/ImportExportTest.sh: replace scripts/ASTImportTest.sh. #12809

scripts/ImportExportTest.sh: replace scripts/ASTImportTest.sh. #12809

Uh oh!

Conversation

aarlt commented Mar 16, 2022 • edited by cameel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

wechman left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarlt commented Mar 28, 2022

Uh oh!

cameel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aarlt commented Mar 28, 2022

Uh oh!

cameel commented Mar 28, 2022

Uh oh!

Uh oh!

aarlt commented Mar 16, 2022 •

edited by cameel

Loading