[tests] Refactor scripts/ASTImportTest.sh.

[aarlt]

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

Fixes #8185

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

[r0qs]

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

[aarlt]

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.

[aarlt]

@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.

[cameel]

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]

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.

[aarlt]

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]

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

[r0qs]

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

[aarlt]

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.

[r0qs]

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]

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]

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]

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]

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.

[aarlt]

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.

[aarlt]

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.

[aarlt]

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.

All requested changes were applied.

[r0qs]

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

[NunoFilipeSantos]

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.