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

[Merged by Bors] - Add ES5 and ES6 Conformance calculation to boa_tester #2690

Closed
wants to merge 6 commits into from

Conversation

ZackMitkin
Copy link
Contributor

@ZackMitkin ZackMitkin commented Mar 18, 2023

This is calculated based the tests es5id or es6id.

Judging by this it's probably not the best method of doing it, but the alternative would require a lot of rework on the boa_tester so that it can pull an older version of the test262 spec which has it's own problems since there's not really an "ES5" only version.

I would think that if we're 100% passing on es5id's then it's safe to assume boa supports es5 (assuming test262's es5id is covering all the es5 cases)

This Pull Request fixes/closes #2629.

It changes the following:

  • Store spec_version in TestResult based on the tests es[6/5]id
  • Count all es5, es6 and their test outcome during TestSuite::run()
  • Print the conformance.

I'm serializing the spec_version outcomes so that it can be displayed in test262 github page. I'd like to work on that too if possible. Let me know if there's anything more I should cover in this PR, I'll gladly do it :)

@codecov
Copy link

codecov bot commented Mar 18, 2023

Codecov Report

Merging #2690 (e6e2c8d) into main (7e6d3c9) will decrease coverage by 0.06%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2690      +/-   ##
==========================================
- Coverage   49.36%   49.30%   -0.06%     
==========================================
  Files         397      396       -1     
  Lines       39568    39776     +208     
==========================================
+ Hits        19533    19612      +79     
- Misses      20035    20164     +129     
Impacted Files Coverage Δ
boa_tester/src/exec/mod.rs 0.00% <0.00%> (ø)
boa_tester/src/main.rs 0.00% <0.00%> (ø)
boa_tester/src/results.rs 0.00% <0.00%> (ø)

... and 28 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

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

It seems that the Test262 comparator fails because the current "main" branch doesn't have the new fields in JSON. I guess they should be optional fields.

@ZackMitkin
Copy link
Contributor Author

It seems that the Test262 comparator fails because the current "main" branch doesn't have the new fields in JSON. I guess they should be optional fields.

Yep, just noticed too, I've just added #[serde(default)] to the TestResult for es5 and es6

Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! I just have a small suggestion for development ease :)

boa_tester/src/main.rs Outdated Show resolved Hide resolved
This is calculated based the tests es5id or es6id
boa_tester/src/main.rs Outdated Show resolved Hide resolved
boa_tester/src/main.rs Outdated Show resolved Hide resolved
boa_tester/src/main.rs Outdated Show resolved Hide resolved
boa_tester/src/exec/mod.rs Outdated Show resolved Hide resolved
@ZackMitkin
Copy link
Contributor Author

I've separated the SuiteResults stats by es-version and all stats. This will break the current CI but I can create a PR to back-fill the previous runs and fix the test262 github page sometime this week

@ZackMitkin ZackMitkin requested a review from jedel1043 March 20, 2023 10:58
@ZackMitkin ZackMitkin requested review from Razican and jedel1043 and removed request for jedel1043 and Razican March 20, 2023 16:33
boa_tester/src/main.rs Outdated Show resolved Hide resolved
@jedel1043
Copy link
Member

Opened #2706 to solve the CI issue. We can merge that PR after we agree on the new data model here and this PR after that.

boa_tester/src/main.rs Outdated Show resolved Hide resolved
boa_tester/src/main.rs Outdated Show resolved Hide resolved
boa_tester/src/exec/mod.rs Outdated Show resolved Hide resolved
this allows us to organize total, passed, ignored, and panic stats by es version
@ZackMitkin ZackMitkin requested a review from jedel1043 March 20, 2023 20:03
Copy link
Member

@jedel1043 jedel1043 left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution! Looks good to me

@jedel1043 jedel1043 added enhancement New feature or request test Issues and PRs related to the tests. labels Mar 20, 2023
@jedel1043 jedel1043 added this to the v0.17.0 milestone Mar 20, 2023
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 good to me! :)

@jedel1043
Copy link
Member

Ok, gonna merge #2706 and then run CI on this.

@ZackMitkin
Copy link
Contributor Author

Ok, gonna merge #2706 and then run CI on this.

Wouldn't it be better to merge your PR into this branch to check CI? Not sure why run it on main...

@jedel1043
Copy link
Member

jedel1043 commented Mar 20, 2023

Wouldn't it be better to merge your PR into this branch to check CI? Not sure why run it on main...

Because #2706 doesn't target main, but gh-pages, so we cannot combine both PRs into one.

@ZackMitkin
Copy link
Contributor Author

Copying the ../gh-pages/test262/refs/heads/main/results.json file to ../results/test262/pull/results.json in order to add the results
Writing the results to ../results/test262/pull...
Error: 
   0: could not write the results to the output JSON file
   1: missing field `a5` at line 1 column 136

No idea what's happening here... I have seen it before though during testing. It worked fine if I wrote to a clean or non-existing dir, but if I tried to overwrite an older schema it'd fail... didn't think serde cared about the overwritten schema...

@jedel1043
Copy link
Member

jedel1043 commented Mar 20, 2023

Yeah, serde is very strict with serializations. Our best option here would be to derive Default for Statistics and add a serde(default) to the new a5 and a6 fields.

@ZackMitkin ZackMitkin requested review from jedel1043 and removed request for Razican March 20, 2023 20:43
boa_tester/src/main.rs Outdated Show resolved Hide resolved
@jedel1043 jedel1043 requested a review from Razican March 20, 2023 22:21
@jedel1043
Copy link
Member

All good now!

Copy link
Member

@jasonwilliams jasonwilliams left a comment

Choose a reason for hiding this comment

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

LGTM
Great contribution!

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 good to me! :)

@jedel1043
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Mar 20, 2023
This is calculated based the tests `es5id` or `es6id`.  

Judging by [this](tc39/test262#1557) it's probably not the best method of doing it, but the alternative would require a lot of rework on the boa_tester so that it can pull an older version of the test262 spec which has it's own problems since there's not really an "ES5" only version.
 
I would think that if we're 100% passing on es5id's then it's safe to assume boa supports es5 (assuming test262's es5id is covering all the es5 cases)

This Pull Request fixes/closes #2629.

It changes the following:
- Store `spec_version` in TestResult based on the tests `es[6/5]id`
- Count all es5, es6 and their test outcome during `TestSuite::run()`
- Print the conformance.

I'm serializing the `spec_version` outcomes so that it can be displayed in test262 github page. I'd like to work on that too if possible. Let me know if there's anything more I should cover in this PR, I'll gladly do it :)

Co-authored-by: jedel1043 <jedel0124@gmail.com>
@bors
Copy link

bors bot commented Mar 20, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Add ES5 and ES6 Conformance calculation to boa_tester [Merged by Bors] - Add ES5 and ES6 Conformance calculation to boa_tester Mar 20, 2023
@bors bors bot closed this Mar 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Collect ES5 conformance information
5 participants