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

Add tests for JsSymbol #3489

Open
jedel1043 opened this issue Nov 29, 2023 · 7 comments
Open

Add tests for JsSymbol #3489

jedel1043 opened this issue Nov 29, 2023 · 7 comments
Assignees
Labels
good first issue Good for newcomers test Issues and PRs related to the tests.

Comments

@jedel1043
Copy link
Member

No description provided.

@jedel1043 jedel1043 added test Issues and PRs related to the tests. good first issue Good for newcomers labels Nov 29, 2023
@gshokanov
Copy link

hey @jedel1043, I'd like to work on this
haven't contributed to the project yet so it will probably take me a few days to figure stuff out

@jedel1043
Copy link
Member Author

Sure! I'll assign it to you. Feel free to ping here or in our discord if you need any guidance.

@gshokanov
Copy link

hey @jedel1043!
I've finally had some time to dive into this but I wanted to ask your opinion on what needs to be tested.

From what I see the main way to test boa_engine is to interpret some JS code and then run assertions. I see that there's a file with some basic tests set up at boa_engine/src/builtins/symbol/tests.rs which doesn't cover a lot, my idea was to add more test cases to this file (like global symbol registry tests with Symbol.for, well known symbols etc.). Is that something that you think makes sense?

I also see that some of well known symbols already have tests in other modules - Symbol.iterator in iterators test module, Symbol.search in regexp test module, etc.; should I keep this structure where it makes sense and add more test cases to let's say a strings test module if a well known symbol is used in string operations?

Lastly, should I add any test cases for JSSymbol itself or should I keep to assertions on executed JS code? Looking through the test files I don't really see examples of running assertions on Rust structs directly skipping JS execution.

@jedel1043
Copy link
Member Author

Is that something that you think makes sense?

Yes! It would be really nice to have symbol tests centralized in that specific file.

should I keep this structure where it makes sense and add more test cases to let's say a strings test module if a well known symbol is used in string operations?

Those tests only use symbols to access the object functions being tested, so I would say those are testing other things more than the symbols themselves.

Lastly, should I add any test cases for JSSymbol itself or should I keep to assertions on executed JS code? Looking through the test files I don't really see examples of running assertions on Rust structs directly skipping JS execution.

Having native JsSymbol tests would be nice. The reason we opened this issue was because we wanted to run Miri tests on JsSymbol, but the Miri interpreter is painfully slow when running JS scripts directly. Having tests using JsSymbol directly would really help in this case. Maybe something like the JsString tests

@gshokanov
Copy link

Sounds good! I'll then work on testing both JS code and native struct.

Maybe something like the JsString tests

I get 404 when opening this link, not sure if the link is broken or I don't have permission to view it.

@jedel1043
Copy link
Member Author

I get 404 when opening this link, not sure if the link is broken or I don't have permission to view it.

That's weird, I tried opening it in a private window and it works...
Anyways, here's a direct link to the file: https://github.com/boa-dev/boa/blob/main/core/engine/src/string/mod.rs

@gshokanov
Copy link

That's weird, I tried opening it in a private window and it works...
Anyways, here's a direct link to the file: https://github.com/boa-dev/boa/blob/main/core/engine/src/string/mod.rs

Yeah, that's weird. Well the direct link works, I'm looking through the tests. I'll see if I can implement something similar for JsSymbol.
Thanks for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers test Issues and PRs related to the tests.
Projects
None yet
Development

No branches or pull requests

2 participants