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

Extract temporal tests from the test262 suite #41

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

jedel1043
Copy link
Member

Just a POC for now, and only generates tests for Duration::add, but theoretically with the correct glue code we could transform all Temporal tests into a better format for us.

Still need to setup the workspace for it.

@jedel1043 jedel1043 added C-enhancement New feature or request C-testing Testing related changes labels Apr 16, 2024
@jedel1043 jedel1043 added this to the 0.1 Blocking milestone Apr 16, 2024
Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

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

Looks awesome! Have some comments, see if they make sense :)

@@ -0,0 +1,26 @@
[package]
Copy link
Member

Choose a reason for hiding this comment

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

We can add the publish = false to the [package] to ensure we don't accidentally publish it :)

Comment on lines +154 to +155
} else if let JsValue::Integer(i) = value {
Ok(*i)
Copy link
Member

Choose a reason for hiding this comment

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

We should also check if it's a float that holds a integer value, otherwise values like 1.0 will be considered not integral.

Also I don't know what are the requirement but even values that are bigger that i32::MAX that don't have a fractional part are considered integral in the spec, so we may want to take care of that.

Comment on lines +126 to +135
let years: i32 = args.get_or_undefined(1).try_js_into(context)?;
let months: i32 = args.get_or_undefined(2).try_js_into(context)?;
let weeks: i32 = args.get_or_undefined(3).try_js_into(context)?;
let days: i32 = args.get_or_undefined(4).try_js_into(context)?;
let hours: i32 = args.get_or_undefined(5).try_js_into(context)?;
let minutes: i32 = args.get_or_undefined(6).try_js_into(context)?;
let seconds: i32 = args.get_or_undefined(7).try_js_into(context)?;
let milliseconds: i32 = args.get_or_undefined(8).try_js_into(context)?;
let microseconds: i32 = args.get_or_undefined(8).try_js_into(context)?;
let nanoseconds: i32 = args.get_or_undefined(9).try_js_into(context)?;
Copy link
Member

Choose a reason for hiding this comment

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

Same with to_integer_if_integral the TryFromJs trait for i32 doesn't seem to be checking for floating point values that are integers.

This seem like an unrelated bug in TryFromJs that should be solved by boa-dev/boa#3761, but maybe we should account for it here, it maybe causing some test to not generate?

Comment on lines +192 to +214
/// Reads the metadata from the input test code.
fn read_metadata(test: &Path) -> Result<MetaData> {
use regex::bytes::Regex;

/// Regular expression to retrieve the metadata of a test.
static META_REGEX: OnceLock<Regex> = OnceLock::new();

let code = fs::read_to_string(test)?;

let yaml = META_REGEX
.get_or_init(|| {
Regex::new(r"/\*\-{3}((?:.|\n)*)\-{3}\*/")
.expect("could not compile metadata regular expression")
})
.captures(code.as_bytes())
.ok_or_eyre("missing metadata for test")?
.get(1)
.map(|m| String::from_utf8_lossy(m.as_bytes()))
.ok_or_eyre("invalid metadata for test")?
.replace('\r', "\n");

serde_yaml::from_str(&yaml).map_err(Into::into)
}
Copy link
Member

@HalidOdat HalidOdat Apr 18, 2024

Choose a reason for hiding this comment

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

I think we have a better version on main: https://github.com/boa-dev/boa/blob/main/tests/tester/src/read.rs#L218-L231

We may want to extract the tester to it's own repository at some point, it's easier now that we have the test262 running nightly on data repository, to avoid having so much duplicate code 😅

@HalidOdat HalidOdat self-requested a review April 18, 2024 04:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement New feature or request C-testing Testing related changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants