boa icon indicating copy to clipboard operation
boa copied to clipboard

Add tests for `JsSymbol`

Open jedel1043 opened this issue 2 years ago • 7 comments

jedel1043 avatar Nov 29 '23 18:11 jedel1043

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

gshokanov avatar Nov 29 '23 22:11 gshokanov

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

jedel1043 avatar Nov 29 '23 23:11 jedel1043

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.

gshokanov avatar Dec 06 '23 23:12 gshokanov

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

jedel1043 avatar Dec 07 '23 00:12 jedel1043

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.

gshokanov avatar Dec 07 '23 00:12 gshokanov

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

jedel1043 avatar Dec 07 '23 00:12 jedel1043

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!

gshokanov avatar Dec 07 '23 00:12 gshokanov