[tests] Refactor scripts/ASTImportTest.sh. #13576

aarlt · 2022-09-28T02:14:34Z

Refactors script/ASTImportTests.sh to make #12834 more readable.

Fixes #8185

please see [tests] Refactor scripts/ASTImportTest.sh. #13576 (comment)

scripts/ASTImportTest.sh

r0qs

Hey, I took a look at the PR, and it looks good, I just made some comments ;)

scripts/ASTImportTest.sh

scripts/common.sh

scripts/ASTImportTest.sh

aarlt · 2022-10-04T20:53:28Z

scripts/ASTImportTest.sh

+        printError "❌ ERROR: AST reimport failed for ${sol_file}"
+        if [[ $ABORT_ON_ERROR == 1 ]]
+        then
+            printError "You can find the files used for this test here: $(pwd)"


I think something like that is useful. It allows to investigate the resulting json files and gives information what was actually done in these tests. so this should allow an easy reproduction.

scripts/common.sh

aarlt · 2022-10-05T19:49:28Z

@cameel @r0qs I'm somehow not sure what to do with #8185. I guess that issue was created, when the script was different. However, right now the only place where we redirect potential errors to /dev/null is where we test a specific source for its compilability. We have especially in test/libsolidity/syntaxTests sources, that are not compilable. So to introduce a mechanism to show these errors would not make much sense from my point of view. For the other steps - e.g. export and import, we will show the compilation errors (compiler's stderr & stdout). Also if there is a mismatch between expected- and obtained output we show the diff. However, I introduced also --exit-on-error, that will stop on the first mismatch and show exactly what commands where used to allow further investigation of potential problems.

scripts/ASTImportTest.sh

scripts/common.sh

cameel · 2022-10-05T20:37:54Z

I'm somehow not sure what to do with #8185. I guess that issue was created, when the script was different. However, right now the only place where we redirect potential errors to /dev/null is where we test a specific source for its compilability.

It wasn't really that much different. In the last revision before the issue was reported I see that only solc output was being sent to /dev/null so basically as it is now.

We have especially in test/libsolidity/syntaxTests sources, that are not compilable. So to introduce a mechanism to show these errors would not make much sense from my point of view.

I think it would still be worthwhile to check why it's not compilable. If it's a compilation error, ignore. But if it's an ICE or a segfault, I'd print the error and exit.

But to detect that, it would be best to compile via Standard JSON since then we can check if we got valid JSON on output. I wouldn't do this in this PR though because it's a bit unrelated. So just post a comment about it in the issue and leave it open.

For the other steps - e.g. export and import, we will show the compilation errors (compiler's stderr & stdout). Also if there is a mismatch between expected- and obtained output we show the diff. However, I introduced also --exit-on-error, that will stop on the first mismatch and show exactly what commands where used to allow further investigation of potential problems.

This is nice and I think it solves part of that issue. I'd still keep it open as I said above though.

aarlt · 2022-10-18T19:39:16Z

I think it would still be worthwhile to check why it's not compilable. If it's a compilation error, ignore. But if it's an ICE or a segfault, I'd print the error and exit.

Hmm.. interesting idea.. I think we should change return codes in solc/main.cpp so that we get 0 on success, 1 on normal failures but 2 on exceptions.

See #13633

cameel

Looks fine overall, but still needs some small corrections.

scripts/common.sh

scripts/ASTImportTest.sh

aarlt · 2022-11-01T17:33:38Z

I think it would still be worthwhile to check why it's not compilable. If it's a compilation error, ignore. But if it's an ICE or a segfault, I'd print the error and exit.

Hmm.. interesting idea.. I think we should change return codes in solc/main.cpp so that we get 0 on success, 1 on normal failures but 2 on exceptions.

See #13633

After #13633 got merged, I change the script accordingly. However, the script will ignore all UnimplementedFeatureError exceptions.

aarlt · 2022-11-02T06:10:26Z

scripts/ASTImportTest.sh

+    # compare expected and obtained ast's
+    if diff_files expected.json obtained.json
+    then
+        echo -n "✅"


After staring on these nice unicode checkmarks for some hours today, I think they should not be used ;)

Maybe we could ignore the print in case of success and only print an error if they are not equivalent?

I will just remove the unicode checkmark. However, I think that CircleCI will terminate jobs that are not generating any output for a specific time. So I will let it still print normal dots.

Yes, the normal dots are fine in my opinion. I was thinking of removing only the checkmarks as you mentioned. And keep the error in case the ast's are not equivalent.

r0qs · 2022-11-02T13:31:53Z

Running the branch locally, I got the following result:

SUCCESS: 1103 tests passed, 0 failed, 1883 could not be compiled (2986 sources total).

Is it expected that 1883 cannot be compiled?

aarlt · 2022-11-03T15:29:52Z

Running the branch locally, I got the following result:
SUCCESS: 1103 tests passed, 0 failed, 1883 could not be compiled (2986 sources total).
Is it expected that 1883 cannot be compiled?

Ah yes! The reason for that the syntax tests. Many of these syntax tests consists of errors to be able to check for specific behaviour of the parser. Maybe we should change that to only use the semantic test cases. But in general the high amount of not compilable tests are expected.

cameel · 2022-11-07T20:07:00Z

Maybe we should change that to only use the semantic test cases.

I think that syntax tests are actually more important for this feature than semantic tests. At least with regard to AST import/export - because they generally use more varied syntax so I'd expect them to cover more corner cases of the AST. For assembly import/export it might actually be the opposite though so ideally we'd have both.

cameel · 2022-11-07T20:43:35Z

scripts/ASTImportTest.sh

+for PARAM in "$@"
+do
+    case "$PARAM" in
+        ast) IMPORT_TEST_TYPE="ast" ;;


I think this should be accepted only once. It's less of a problem when there's one possible value but later this approach will allow things like scripts/ASTImportTest.sh --exit-on-error evm-assembly ast, which is very misleading because it looks as if it would run both sets of tests.

I added a simple function check_import_test_type_unset that is called before setting IMPORT_TEST_TYPE. This should solve the problem mentioned here.

scripts/ASTImportTest.sh

aarlt · 2022-11-09T21:52:55Z

Running the branch locally, I got the following result:
SUCCESS: 1103 tests passed, 0 failed, 1883 could not be compiled (2986 sources total).
Is it expected that 1883 cannot be compiled?
Ah yes! The reason for that the syntax tests. Many of these syntax tests consists of errors to be able to check for specific behaviour of the parser. Maybe we should change that to only use the semantic test cases. But in general the high amount of not compilable tests are expected.

@r0qs @cameel The original script did not invoke the code generator. This was changed in this PR. Thats why many of the syntax tests were treated as non-compilable. See #13576 (comment). I corrected this now.

cameel

I still have some suggestions but they're all cosmetic so tentatively approving.

scripts/ASTImportTest.sh

aarlt · 2022-11-11T03:20:53Z

I also added now another change in scripts/common.sh. I noticed that the printStackTrace function was not working correctly under macOS. The cut command there is not the gnu version (similar to readlink). This version does not support e.g. --delimiter. However, on macOS it will now use gcut instead of cut.

scripts/common.sh

scripts/ASTImportTest.sh

All requested changes were applied.

r0qs

Please, fix this: #13576 (comment) and we are good to go :)

Co-authored-by: Kamil Śliwak <[email protected]>

NunoFilipeSantos · 2022-11-24T10:52:28Z

Please, fix this: #13576 (comment) and we are good to go :)

I just applied it. 😛 Didn't see the need to wait.
Now waiting for the tests to pass.

* [tests] Refactor scripts/ASTImportTest.sh. Co-authored-by: Nuno Santos <[email protected]> Co-authored-by: Kamil Śliwak <[email protected]>

aarlt mentioned this pull request Sep 28, 2022

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

Closed

aarlt force-pushed the import-export-script-refactoring branch from e7f6556 to 47a5775 Compare September 28, 2022 02:20

aarlt commented Sep 28, 2022

View reviewed changes

scripts/ASTImportTest.sh Outdated Show resolved Hide resolved

cameel requested a review from r0qs September 28, 2022 11:27

aarlt added the refactor label Sep 28, 2022

r0qs requested changes Oct 3, 2022

View reviewed changes

aarlt commented Oct 4, 2022

View reviewed changes

scripts/ASTImportTest.sh Outdated Show resolved Hide resolved

aarlt commented Oct 4, 2022

View reviewed changes

r0qs reviewed Oct 5, 2022

View reviewed changes

scripts/common.sh Show resolved Hide resolved

aarlt force-pushed the import-export-script-refactoring branch 2 times, most recently from 0c2ee2a to 73b5b88 Compare October 5, 2022 19:39

aarlt force-pushed the import-export-script-refactoring branch from 73b5b88 to c13aa5b Compare October 5, 2022 19:50

aarlt commented Oct 5, 2022

View reviewed changes

scripts/ASTImportTest.sh Outdated Show resolved Hide resolved

aarlt commented Oct 5, 2022

View reviewed changes

scripts/ASTImportTest.sh Outdated Show resolved Hide resolved

cameel reviewed Oct 5, 2022

View reviewed changes

scripts/ASTImportTest.sh Outdated Show resolved Hide resolved

scripts/ASTImportTest.sh Outdated Show resolved Hide resolved

scripts/common.sh Outdated Show resolved Hide resolved

scripts/common.sh Outdated Show resolved Hide resolved

scripts/common.sh Outdated Show resolved Hide resolved

aarlt force-pushed the import-export-script-refactoring branch from c13aa5b to 8597351 Compare October 5, 2022 20:33

aarlt force-pushed the import-export-script-refactoring branch from 8597351 to dabe9b4 Compare October 18, 2022 19:42

aarlt mentioned this pull request Oct 18, 2022

[solc] Exit code 2 for exceptions. #13633

Merged

cameel reviewed Oct 21, 2022

View reviewed changes

scripts/common.sh Outdated Show resolved Hide resolved

scripts/ASTImportTest.sh Outdated Show resolved Hide resolved

scripts/ASTImportTest.sh Outdated Show resolved Hide resolved

scripts/ASTImportTest.sh Outdated Show resolved Hide resolved

aarlt force-pushed the import-export-script-refactoring branch from dabe9b4 to c6c82b4 Compare November 1, 2022 18:19

aarlt requested review from r0qs and cameel November 1, 2022 18:20

aarlt mentioned this pull request Nov 2, 2022

Add support to import evm assembly json (updated). #13673

Merged

6 tasks

aarlt commented Nov 2, 2022

View reviewed changes

cameel mentioned this pull request Nov 7, 2022

AST reimport fails for inline assembly with uninitialized variable declarations #13684

Open

cameel reviewed Nov 7, 2022

View reviewed changes

aarlt force-pushed the import-export-script-refactoring branch from 7e82fc1 to 357d81c Compare November 9, 2022 21:50

NunoFilipeSantos assigned aarlt Nov 10, 2022

cameel previously approved these changes Nov 10, 2022

View reviewed changes

aarlt dismissed cameel’s stale review via deaddec November 11, 2022 03:24

aarlt force-pushed the import-export-script-refactoring branch 2 times, most recently from deaddec to dcfc59e Compare November 11, 2022 03:32

aarlt requested review from r0qs and cameel November 12, 2022 00:29

cameel reviewed Nov 21, 2022

View reviewed changes

scripts/common.sh Outdated Show resolved Hide resolved

[tests] Refactor scripts/ASTImportTest.sh.

ab706e3

aarlt force-pushed the import-export-script-refactoring branch from dcfc59e to ab706e3 Compare November 23, 2022 13:08

cameel previously approved these changes Nov 23, 2022

View reviewed changes

scripts/ASTImportTest.sh Outdated Show resolved Hide resolved

r0qs previously approved these changes Nov 24, 2022

View reviewed changes

Update scripts/ASTImportTest.sh

17cfdb1

Co-authored-by: Kamil Śliwak <[email protected]>

NunoFilipeSantos dismissed stale reviews from r0qs and cameel via 17cfdb1 November 24, 2022 10:51

r0qs self-requested a review November 24, 2022 12:52

r0qs approved these changes Nov 24, 2022

View reviewed changes

cameel approved these changes Nov 24, 2022

View reviewed changes

cameel merged commit fe68200 into develop Nov 24, 2022

cameel deleted the import-export-script-refactoring branch November 24, 2022 15:16

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tests] Refactor scripts/ASTImportTest.sh. #13576

[tests] Refactor scripts/ASTImportTest.sh. #13576

aarlt commented Sep 28, 2022 •

edited

Loading

r0qs left a comment

aarlt Oct 4, 2022

aarlt commented Oct 5, 2022

cameel commented Oct 5, 2022 •

edited

Loading

aarlt commented Oct 18, 2022 •

edited

Loading

cameel left a comment

aarlt commented Nov 1, 2022

aarlt Nov 2, 2022

r0qs Nov 2, 2022

aarlt Nov 3, 2022

r0qs Nov 4, 2022

r0qs commented Nov 2, 2022

aarlt commented Nov 3, 2022

cameel commented Nov 7, 2022

cameel Nov 7, 2022

aarlt Nov 9, 2022

aarlt commented Nov 9, 2022

cameel left a comment

aarlt commented Nov 11, 2022

r0qs left a comment

NunoFilipeSantos commented Nov 24, 2022 •

edited

Loading

[tests] Refactor scripts/ASTImportTest.sh. #13576

[tests] Refactor scripts/ASTImportTest.sh. #13576

Conversation

aarlt commented Sep 28, 2022 • edited Loading

r0qs left a comment

Choose a reason for hiding this comment

aarlt Oct 4, 2022

Choose a reason for hiding this comment

aarlt commented Oct 5, 2022

cameel commented Oct 5, 2022 • edited Loading

aarlt commented Oct 18, 2022 • edited Loading

cameel left a comment

Choose a reason for hiding this comment

aarlt commented Nov 1, 2022

aarlt Nov 2, 2022

Choose a reason for hiding this comment

r0qs Nov 2, 2022

Choose a reason for hiding this comment

aarlt Nov 3, 2022

Choose a reason for hiding this comment

r0qs Nov 4, 2022

Choose a reason for hiding this comment

r0qs commented Nov 2, 2022

aarlt commented Nov 3, 2022

cameel commented Nov 7, 2022

cameel Nov 7, 2022

Choose a reason for hiding this comment

aarlt Nov 9, 2022

Choose a reason for hiding this comment

aarlt commented Nov 9, 2022

cameel left a comment

Choose a reason for hiding this comment

aarlt commented Nov 11, 2022

r0qs left a comment

Choose a reason for hiding this comment

NunoFilipeSantos commented Nov 24, 2022 • edited Loading

aarlt commented Sep 28, 2022 •

edited

Loading

cameel commented Oct 5, 2022 •

edited

Loading

aarlt commented Oct 18, 2022 •

edited

Loading

NunoFilipeSantos commented Nov 24, 2022 •

edited

Loading