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

Fix display for nodes #1312

Merged
merged 46 commits into from
Jun 18, 2021
Merged

Fix display for nodes #1312

merged 46 commits into from
Jun 18, 2021

Conversation

macmv
Copy link
Contributor

@macmv macmv commented Jun 9, 2021

This Pull Request fixes/closes #1311

This is a collection of fixes for the fmt::Display functions on various nodes. With this PR, you can print a StatementList to stdout in nearly the same format as the source code.

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for these contributions!! This will look much better now. Could we add some tests to make sure new changes don't break it?

@Razican Razican added bug Something isn't working parser Issues surrounding the parser labels Jun 9, 2021
@Razican Razican added this to the v0.13.0 milestone Jun 9, 2021
@Razican Razican added the cli Issues and PRs related to the Boa command line interface. label Jun 9, 2021
@macmv
Copy link
Contributor Author

macmv commented Jun 9, 2021

Tests take a long time to write. I'm going to work on this later tonight. I put all the tests I've written so far in the tests.rs file in each node directory (for example, syntax/ast/node/call/tests.rs). There is also a utility function for testing all of these, which is at the bottom of syntax/ast/node/mod.rs. I wasn't really sure where else to put it, and I made sure its #[cfg(test)].

@Razican
Copy link
Member

Razican commented Jun 10, 2021

Tests take a long time to write. I'm going to work on this later tonight. I put all the tests I've written so far in the tests.rs file in each node directory (for example, syntax/ast/node/call/tests.rs). There is also a utility function for testing all of these, which is at the bottom of syntax/ast/node/mod.rs. I wasn't really sure where else to put it, and I made sure its #[cfg(test)].

Something that could simplify writing the tests would be to add a folder with some code snippets to test. That way, you can just load each of them in a test and compare the outputs.

@macmv
Copy link
Contributor Author

macmv commented Jun 11, 2021

I think setting up a loader for a bunch of files would just take more time. The most time-consuming part of these tests was just setting up the testing function, and re-writing my previous tests.

@Razican
Copy link
Member

Razican commented Jun 14, 2021

Test262 conformance changes:

Test result master count PR count difference
Total 78,897 78,897 0
Passed 26,857 26,858 +1
Ignored 15,628 15,628 0
Failed 36,412 36,411 -1
Panics 0 0 0
Conformance 34.04% 34.04% +0.00%
Fixed tests:
test/built-ins/Map/prototype/set/append-new-values-normalizes-zero-key.js (previously Failed)

@macmv
Copy link
Contributor Author

macmv commented Jun 14, 2021

Alright, I've finished up the tests. The world is beautiful, and everything passes, and a lot of bugs were fixed. I think I covered everything, but tarpaulin is hard to use, so I am not entirely sure that I didn't miss something.

Copy link
Contributor

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor thing to avoid compiler warnings the rest looks great.

boa/src/syntax/ast/node/operator/tests.rs Outdated Show resolved Hide resolved
Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is almost ready. Great work!!! Check my comment and @RageKnify's :)

boa/src/syntax/ast/node/mod.rs Outdated Show resolved Hide resolved
@macmv
Copy link
Contributor Author

macmv commented Jun 18, 2021

I've fixed both of those issues. Thank you for pointing those out. Also, cargo fix didn't do anything except remove that unused import, so I don't think there are any other warnings I created.

@Razican Razican merged commit 057cbb1 into boa-dev:master Jun 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working cli Issues and PRs related to the Boa command line interface. parser Issues surrounding the parser
Projects
None yet
Development

Successfully merging this pull request may close these issues.

StatementList display is broken
3 participants