#3696 - tc39-test262 Crate #3708

PawelJastrzebski · 2024-02-29T13:45:49Z

This Pull Request closes #3696.

It changes the following:

transfers boa_tester parser logic to tools/tc39-test262 crate

codecov · 2024-03-07T00:12:11Z

Codecov Report

Attention: Patch coverage is 0% with 170 lines in your changes are missing coverage. Please review.

Project coverage is 47.37%. Comparing base (6ddc2b4) to head (154f7a1).
Report is 80 commits behind head on main.

❗ Current head 154f7a1 differs from pull request most recent head 5e85673. Consider uploading reports for the commit 5e85673 to get more accurate results

Files	Patch %	Lines
tools/tc39-test262/src/git.rs	0.00%	58 Missing ⚠️
tools/tc39-test262/src/test_flags.rs	0.00%	55 Missing ⚠️
tools/tc39-test262/src/structs.rs	0.00%	26 Missing ⚠️
tools/tc39-test262/src/test_files.rs	0.00%	20 Missing ⚠️
tools/tc39-test262/src/lib.rs	0.00%	6 Missing ⚠️
tools/tc39-test262/src/read.rs	0.00%	4 Missing ⚠️
tools/tc39-test262/src/edition.rs	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3708      +/-   ##
==========================================
+ Coverage   47.24%   47.37%   +0.12%     
==========================================
  Files         476      461      -15     
  Lines       46892    44846    -2046     
==========================================
- Hits        22154    21245     -909     
+ Misses      24738    23601    -1137

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

PawelJastrzebski

Merge comments

PawelJastrzebski · 2024-07-04T11:52:41Z

tests/tester/src/exec/mod.rs

@@ -220,54 +220,6 @@ impl RunTest<OptimizerOptions, TestResult> for Test {
        }
    }

-    /// Creates the test result from the outcome and message.
-    fn create_result<S: Into<Box<str>>>(


Moved to helper function (Out of Trait)

PawelJastrzebski · 2024-07-04T11:56:04Z

tools/tc39-test262/src/edition.rs

@@ -87,9 +93,13 @@ static FEATURE_EDITION: phf::Map<&'static str, SpecEdition> = phf::phf_map! {
    // https://github.com/tc39/proposal-set-methods
    "set-methods" => SpecEdition::ESNext,

-    // Regular Expression Pattern Modifiers
-    // https://github.com/tc39/proposal-regexp-modifiers
-    "regexp-modifiers" => SpecEdition::ESNext,


Duplicate in line 76

jedel1043

Thank you for all the work!

It's been several days going back and forth on this PR about whether to put the feature to editions map into the new crate or the tester.

On the one hand, features are part of the test262 suite and we may include them to also get the edition of a test. On the other hand, if we include them, we would need to keep the crate pretty much updated with every new commit to the test262 repo, which would be a pretty big maintenance work for us.

I'm tending towards leaving the editions on the tester, since I don't see many use cases for it aside from engine tests. What do you think?

PawelJastrzebski · 2024-07-22T09:46:21Z

I wonder if would be ok for tests that contains unknown features set edition to ESNext as a fallback option. I guess in that case they could be marked as ignored as well.

Thinking long term:

test262 repo contains list of features features.txt.
We could read from that to find out if missing feature pass through the proposal stage, and only then break the parsing process with an error.

jedel1043 · 2024-07-22T18:36:12Z

I wonder if would be ok for tests that contains unknown features set edition to ESNext as a fallback option. I guess in that case they could be marked as ignored as well.

That would also be a good option for the short-term.

On the long term, I'm trying to push for adding this metadata to test262 (tc39/test262#4161). This would essentially remove the need for the map, probably making it a HashMap<String, u8> in the future.

PawelJastrzebski · 2024-08-22T02:55:50Z

tools/tc39-test262/src/edition.rs

@@ -39,12 +39,6 @@ static FEATURE_EDITION: phf::Map<&'static str, SpecEdition> = phf::phf_map! {
    // https://github.com/tc39/proposal-json-modules
    "json-modules"  => SpecEdition::ESNext,

-    // https://github.com/tc39/proposal-source-phase-imports


PawelJastrzebski · 2024-08-22T03:09:36Z

Temporally fallback to ESNext while pending on features map on tc39/test262#4161.

@jedel1043 Could you please take a look on this:
391ac73#diff-91b6a7ace0b3edddaae2e4339b9cc565ce1cb47e735f2ad88805bcb533f3343aL390

I guess with that change we could merge this PR and implement features map parser later.

jedel1043

First pass of reviews. It's starting to look really nice!

jedel1043 · 2024-08-24T16:06:27Z

tests/tester/src/exec/mod.rs

+pub(crate) trait RunTest<TO, TR> {
+    /// Runs the test.
+    fn run(&self, harness: &Harness, verbose: u8, optimizer_options: TO, console: bool) -> TR;
+
+    /// Runs the test once, in strict or non-strict mode
+    fn run_once(
+        &self,
+        harness: &Harness,
+        strict: bool,
+        verbose: u8,
+        optimizer_options: OptimizerOptions,
+        console: bool,
+    ) -> TestResult;
+}
+
+pub(crate) trait RunTestSuite<TO, TR> {
+    /// Runs the test suite.
+    fn run(
+        &self,
+        harness: &Harness,
+        verbose: u8,
+        parallel: bool,
+        max_edition: SpecEdition,
+        optimizer_options: OptimizerOptions,
+        console: bool,
+    ) -> SuiteResult;
+}


Do we need these to be generic traits? They could also be simple traits without generics, since they're only used internally.

You are right, we can remove generics.

jedel1043 · 2024-08-24T16:11:40Z

tools/tc39-test262/Cargo.toml

@@ -0,0 +1,24 @@
+[package]
+name = "tc39-test262"


Is test262 available in crates.io? That could potentially be a better name for the crate.

looks like test262 it's available.

jedel1043 · 2024-08-24T17:03:34Z

tools/tc39-test262/src/edition.rs

@@ -292,7 +290,7 @@ static FEATURE_EDITION: phf::Map<&'static str, SpecEdition> = phf::phf_map! {
    clap::ValueEnum,
 )]
 #[repr(u8)]
-pub(crate) enum SpecEdition {
+pub enum SpecEdition {


To prepare for the test262 changes, this should probably become an open enum instead.

pub struct SpecEdition(u8); impl SpecEdition { const ES5: SpecEdition = SpecEdition(5); const ES8: SpecEdition = SpecEdition(8); }

jedel1043 · 2024-08-24T19:26:31Z

tools/tc39-test262/src/lib.rs

+pub fn clone_test262(commit: Option<&str>, verbose: u8) -> color_eyre::Result<()> {
+    const TEST262_REPOSITORY: &str = "https://github.com/tc39/test262";
+    git::clone(
+        TEST262_DIRECTORY,
+        TEST262_REPOSITORY,
+        &"origin/main",
+        commit,
+        verbose,
+    )
+}


I'm not sure if we should expose a clone_repo function. IMO, we should provide a test262 function that takes a struct of options and returns the whole suite, optionally cloning the repo if a path to a valid repo is not provided.

Okey, that seams reasonable to me.

After longer consideration and refactor I not sure if reading and optionally cloning logic should be in the same function (double responsibility).

it would add more complexity to read(...) fn signature as we would have to provide extra information (struct) on each read level (harness, suite, test).

if we provide it only on the suite level api in that form can be confusing and inconsistent.

Usage in main:

in this example if path to suite would be a direct path to test (.js) test262 would skip clone and return error, causing inconsistent cli behavior.

@jedel1043 please let me know what do you think about it. Maybe I'm missing something and there is a better solution to make it done.

We don't need the public read to be the same as the read used internally. I'd expect something like having a test262::read that does all the fetch git logic, then a private test262::read_from_dict that is called by test262::read and does the reading tests logic. Also, I'd avoid exposing pub read functions for the other components.

@jedel1043 Thanks for clarification.

Please take a look at this changes and let me know if there are closer to the expected result.
027fbb4?diff=split&w=0

Still some work is needed, but I want to make sure that this is the desired direction.

jedel1043 · 2024-08-24T19:29:56Z

tools/tc39-test262/src/read.rs


 /// Reads the Test262 defined bindings.
-pub(super) fn read_harness(test262_path: &Path) -> Result<Harness> {
+pub fn read_harness(test262_path: &Path) -> Result<Harness> {


Again, I think all of these should just be private, and the user should be able to explore the repo using methods in Test, TestSuite, and friends.

jedel1043 · 2024-08-24T19:32:45Z

tools/tc39-test262/src/test_files.rs

+
+impl Test {
+    /// Creates a new test.
+    pub fn new<N, C>(name: N, path: C, metadata: MetaData) -> color_eyre::Result<Self>


We should not use eyre for error handling. We should have an Error enum that people can match on.

PawelJastrzebski

Thanks for review @jedel1043

PawelJastrzebski · 2024-08-26T15:58:49Z

tools/tc39-test262/Cargo.toml

@@ -0,0 +1,24 @@
+[package]
+name = "tc39-test262"


looks like test262 it's available.

PawelJastrzebski · 2024-08-26T16:18:11Z

tools/tc39-test262/src/lib.rs

+pub fn clone_test262(commit: Option<&str>, verbose: u8) -> color_eyre::Result<()> {
+    const TEST262_REPOSITORY: &str = "https://github.com/tc39/test262";
+    git::clone(
+        TEST262_DIRECTORY,
+        TEST262_REPOSITORY,
+        &"origin/main",
+        commit,
+        verbose,
+    )
+}


Okey, that seams reasonable to me.

PawelJastrzebski · 2024-08-26T17:15:33Z

tests/tester/src/exec/mod.rs

+pub(crate) trait RunTest<TO, TR> {
+    /// Runs the test.
+    fn run(&self, harness: &Harness, verbose: u8, optimizer_options: TO, console: bool) -> TR;
+
+    /// Runs the test once, in strict or non-strict mode
+    fn run_once(
+        &self,
+        harness: &Harness,
+        strict: bool,
+        verbose: u8,
+        optimizer_options: OptimizerOptions,
+        console: bool,
+    ) -> TestResult;
+}
+
+pub(crate) trait RunTestSuite<TO, TR> {
+    /// Runs the test suite.
+    fn run(
+        &self,
+        harness: &Harness,
+        verbose: u8,
+        parallel: bool,
+        max_edition: SpecEdition,
+        optimizer_options: OptimizerOptions,
+        console: bool,
+    ) -> SuiteResult;
+}


You are right, we can remove generics.

crate extract work in progress

e3a9952

PawelJastrzebski marked this pull request as draft February 29, 2024 13:45

extracted tc39-test262 crate

154f7a1

PawelJastrzebski added 2 commits March 16, 2024 11:00

manual tests

80a129b

merge main

5e85673

PawelJastrzebski marked this pull request as ready for review March 16, 2024 10:24

PawelJastrzebski changed the title ~~#3696 - WIP~~ #3696 - tc39-test262 Crate Mar 16, 2024

resolve conflicts

4d8bc61

PawelJastrzebski commented Jul 4, 2024

View reviewed changes

PawelJastrzebski mentioned this pull request Jul 4, 2024

Split boa_tester into crates #3696

Open

jedel1043 reviewed Jul 21, 2024

View reviewed changes

PawelJastrzebski added 2 commits August 22, 2024 04:08

Merge branch 'main' into issue-3696

e535b90

SpecEdition - Temporally fallback to ESNext for unknown features

391ac73

PawelJastrzebski commented Aug 22, 2024

View reviewed changes

jedel1043 requested changes Aug 24, 2024

View reviewed changes

PawelJastrzebski commented Aug 26, 2024

View reviewed changes

PawelJastrzebski added 6 commits September 13, 2024 12:01

Merge remote-tracking branch 'origin/main' into issue-3696

673e23c

removed redundand generics

746320b

rename tc39_test262 to test262, refactor SpecEdition to open enum

29e18d2

test262 Error

d4f55fc

test262 hermetization

6dc72ca

WIP - test262:read fn

027fbb4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#3696 - tc39-test262 Crate #3708

#3696 - tc39-test262 Crate #3708

PawelJastrzebski commented Feb 29, 2024 •

edited

Loading

codecov bot commented Mar 7, 2024 •

edited

Loading

PawelJastrzebski left a comment

PawelJastrzebski Jul 4, 2024

PawelJastrzebski Jul 4, 2024

jedel1043 left a comment

PawelJastrzebski commented Jul 22, 2024

jedel1043 commented Jul 22, 2024

PawelJastrzebski Aug 22, 2024

PawelJastrzebski commented Aug 22, 2024

jedel1043 left a comment

jedel1043 Aug 24, 2024

PawelJastrzebski Aug 26, 2024

jedel1043 Aug 24, 2024

PawelJastrzebski Aug 26, 2024

jedel1043 Aug 24, 2024

jedel1043 Aug 24, 2024

PawelJastrzebski Aug 26, 2024

PawelJastrzebski Sep 13, 2024

jedel1043 Sep 16, 2024 •

edited

Loading

PawelJastrzebski Sep 20, 2024

jedel1043 Aug 24, 2024 •

edited

Loading

jedel1043 Aug 24, 2024

PawelJastrzebski left a comment

PawelJastrzebski Aug 26, 2024

PawelJastrzebski Aug 26, 2024

PawelJastrzebski Aug 26, 2024

#3696 - tc39-test262 Crate #3708

Are you sure you want to change the base?

#3696 - tc39-test262 Crate #3708

Conversation

PawelJastrzebski commented Feb 29, 2024 • edited Loading

codecov bot commented Mar 7, 2024 • edited Loading

Codecov Report

PawelJastrzebski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 left a comment

Choose a reason for hiding this comment

PawelJastrzebski commented Jul 22, 2024

jedel1043 commented Jul 22, 2024

Choose a reason for hiding this comment

PawelJastrzebski commented Aug 22, 2024

jedel1043 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 Sep 16, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedel1043 Aug 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PawelJastrzebski left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PawelJastrzebski commented Feb 29, 2024 •

edited

Loading

codecov bot commented Mar 7, 2024 •

edited

Loading

jedel1043 Sep 16, 2024 •

edited

Loading

jedel1043 Aug 24, 2024 •

edited

Loading