-
-
Notifications
You must be signed in to change notification settings - Fork 406
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 by Bors] - Migrate tests to new test API #2619
Conversation
d62641a
to
de8f88b
Compare
Test262 conformance changes
|
Codecov Report
@@ Coverage Diff @@
## main #2619 +/- ##
==========================================
+ Coverage 49.39% 49.56% +0.17%
==========================================
Files 387 388 +1
Lines 39278 39275 -3
==========================================
+ Hits 19400 19468 +68
+ Misses 19878 19807 -71
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
||
let main_timer = Profiler::global().start_event("Main", "Main"); | ||
/// Runs `source`, panickinf if the execution throws. | ||
pub(crate) fn run(source: impl Into<Cow<'static, str>>) -> Self { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for using Cow
here instead of just the static string? Are these test strings ever being mutated?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are some tests that programmatically generate the tested scripts:
boa/boa_engine/src/tests/mod.rs
Lines 429 to 431 in 6c0482e
run_test(cases.into_iter().map(|(case, msg)| { | |
TestAction::assert_native_error(format!("'use strict'; {case}"), ErrorKind::Syntax, msg) | |
})); |
The other option was to take a
String
instead of a &'static str
, but that would make almost all tests slower, so I opted for taking a Cow
instead.
boa_engine/src/vm/tests.rs
Outdated
typeof a; | ||
"#; | ||
assert_eq!(&exec(typeof_object), "\"string\""); | ||
run_test([TestAction::assert_eq( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these tests are only passing 1 item in the array, do we want an API which can handle just a single test?
The name run_test doens't sound right when it's designed to take in an array of tests. But I understand this is just bikeshedding.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
run_test_actions
maybe? Or could be even run_actions
, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that name would be describing better what is happening yeah. (Even though it is longer)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these tests are only passing 1 item in the array, do we want an API which can handle just a single test?
Thought about it, but I didn't think it would be worth increasing the complexity of the API just to save two ([
and ]
) keystrokes...
... but now that I think about it, we could implement IntoIterator
for TestAction
, which doesn't increase our API complexity, though it makes run_test_actions(TestAction::exec(..))
look a bit weird.
Looks good so far, but improvement to what we had before! |
6c0482e
to
e909f81
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The tests look much cleaner now! Great work @jedel1043
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice improvement!
Gonna merge this since I've already resolved Jason's comments, and It'll be better to merge this ASAP to avoid merge conflicts. |
bors r+ |
This PR migrates our entire test suite to our new testing API. It changes the following: - Migrates tests to new test API. - Cleans up the API to be a bit more descriptive and maintainable. - Prettifies our test failure display to show the failed scripts. - Splits our massive `tests.rs` file into smaller sub-suites. Example output of a failing test: ![image](https://user-images.githubusercontent.com/38230983/221502567-9e219371-b4ab-49d0-b42b-94a9b1a9c002.png)
Pull request successfully merged into main. Build succeeded:
|
This PR migrates our entire test suite to our new testing API.
It changes the following:
tests.rs
file into smaller sub-suites.Example output of a failing test: