Skip to content

Got CI working again (Correctly identifies everything that is failing) #14

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

Merged
merged 5 commits into from
Jul 9, 2020

Conversation

torch2424
Copy link
Collaborator

@torch2424 torch2424 commented Jul 9, 2020

This PR finalizes getting all of the CI to work (in terms of catching failing tests).

These fixes come from mostly checking the build logs in: https://github.com/DuncanUszkay1/assemblyscript/runs/846787874

This will be organized by the things that are fixed

Features testing

See: https://github.com/DuncanUszkay1/assemblyscript/runs/846789583

This is fixed by recreating tests/compiler/features/reference-types.untouched.wat. Which is now updated to use closures.

Loader Testing

See: https://github.com/DuncanUszkay1/assemblyscript/runs/846789746

This is actually working correctly, with an actionable error. It has to do with exporting a closure: https://github.com/DuncanUszkay1/assemblyscript/blob/master/lib/loader/tests/assembly/index.ts#L62

Bootstrap Testing

See: https://github.com/DuncanUszkay1/assemblyscript/runs/846789713

This is also working correctly, but finds a bug that I started fixing, that I think may be worth including in this PR.

This attempts to do a self-compilation of AssemblyScript, and works for most of the program, but breaks on a recursive closure you can find here: https://github.com/DuncanUszkay1/assemblyscript/blob/master/src/flow.ts#L1398

And some screenshots to prove it:

Before fix:
Screenshot from 2020-07-08 16-26-31

After fixing the name conflict:
Screenshot from 2020-07-08 16-23-15

Still broken:
Screenshot from 2020-07-08 16-22-50

Hanging tests on validation:

This is apparent as of: https://github.com/DuncanUszkay1/assemblyscript/runs/846789647 Which hung for 5 hours 😂

The fix is in this PR at: cli/asc.js where a return is incorectly placed in a callback. This fix should be reflected in AS main as well, and I'll do that tomorrow 😄

EDIT: Opened here: AssemblyScript#1383 (comment)

In conclusion

And this leaves us with this remaining CI result:

Screenshot from 2020-07-08 17-52-24

So now we have a much more clear guide on what/how things need to be fixed in closures to get tests passing again 😄 👍

@torch2424 torch2424 added bug Something isn't working enhancement New feature or request labels Jul 9, 2020
@torch2424 torch2424 requested a review from DuncanUszkay1 July 9, 2020 01:10
@torch2424 torch2424 self-assigned this Jul 9, 2020
@torch2424
Copy link
Collaborator Author

Also, I went ahead and commited re-creating all the test .wat since that is compared in the tests and will fail if it doesnt match (which is what we expect since we are changing this 😄 )

@torch2424
Copy link
Collaborator Author

@DuncanUszkay1 I went ahead and opened the PR for the hanging tests on AS Main here: AssemblyScript#1383 (comment)

I still think we should merge this though, since we'll have to sync with AS eventually anyways, and if the PR gets merged as-is, shouldn't cause any issues 😄

@DuncanUszkay1
Copy link
Owner

Merge this PR? Seems like the stated goal of the PR isn't done yet given the failing tests though- or am I confused?

@torch2424
Copy link
Collaborator Author

@DuncanUszkay1

Merge this PR? Seems like the stated goal of the PR isn't done yet given the failing tests though- or am I confused?

Oh yes! We can merge! The goal of this PR was to get the tests running in full again (instead of failing / hanging early on) 😄 In previous builds the tests weren't actually running due to the various reasons that I fixed in previous PRs and this one.

This PR now shows we are currently failing 6 compiler tests, so next would be to open a PR to start fixing our closures implementation so we start passing more tests 😄

@DuncanUszkay1 DuncanUszkay1 merged commit bfab2bd into master Jul 9, 2020
@DuncanUszkay1 DuncanUszkay1 deleted the fix-remaining-small-ci-issues branch July 9, 2020 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants