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

Add missing 'unscopables' to Array.prototype[@@unscopables] #3111

Merged
merged 1 commit into from Jul 9, 2023

Conversation

dirkdev98
Copy link
Contributor

This also includes the variants for toSorted, toSpliced and toReversed even though they are not implemented yet. This doesn't have any effect on the runtime since with is not implemented to use Symbol.unscopables.

@dirkdev98
Copy link
Contributor Author

dirkdev98 commented Jul 5, 2023

Test262 conformance changes

Test result main count PR count difference
Total 94,858 94,858 0
Passed 74,691 74,693 +2
Ignored 18,828 18,828 0
Failed 1,339 1,337 -2
Panics 0 0 0
Conformance 78.74% 78.74% +0.00%
Fixed tests (2):
test/built-ins/Array/prototype/Symbol.unscopables/array-find-from-last.js [strict mode] (previously Failed)
test/built-ins/Array/prototype/Symbol.unscopables/array-find-from-last.js (previously Failed)

@codecov
Copy link

codecov bot commented Jul 5, 2023

Codecov Report

Merging #3111 (76f8dd1) into main (9b1f2f4) will increase coverage by 0.25%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #3111      +/-   ##
==========================================
+ Coverage   50.42%   50.67%   +0.25%     
==========================================
  Files         444      444              
  Lines       45877    42621    -3256     
==========================================
- Hits        23132    21599    -1533     
+ Misses      22745    21022    -1723     
Impacted Files Coverage Δ
boa_engine/src/builtins/array/mod.rs 76.32% <100.00%> (-0.76%) ⬇️

... and 187 files with indirect coverage changes

@HalidOdat HalidOdat added enhancement New feature or request builtins PRs and Issues related to builtins/intrinsics labels Jul 5, 2023
@jedel1043
Copy link
Member

Haven't forgot about reviewing this :)

We just have to investigate the regressions first, but the PR looks great!

@Razican
Copy link
Member

Razican commented Jul 9, 2023

Haven't forgot about reviewing this :)

We just have to investigate the regressions first, but the PR looks great!

It seems that the regressions are previous false positives, since I can also see them fail in #3094, which doesn't seem related.

@dirkdev98
Copy link
Contributor Author

No worries!

We just have to investigate the regressions first, but the PR looks great!

I assume you mean the failing test/intl402/ListFormat/constructor/constructor/locales-valid.js test262 test right?
Is there an issue or discussion somewhere about this already or should I create one?

@dirkdev98
Copy link
Contributor Author

I added the comment with test conformance changes based on a local result.

cargo run --release --bin boa_tester -- run -v -o ./local/changes
./target/release/boa_tester compare ../boa-gh-pages/test262/refs/heads/main/latest.json ./local/changes/latest.json  -m

where boa-gh-pages is the latest gh-pages branch.

CI seems to be fine https://github.com/boa-dev/boa/actions/runs/5468021068/jobs/9955094733#step:11:80. So something is up for me locally. I'll clean my environment and try to figure out whats going on and create an issue separately with local details when I can reproduce it afterwards.

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.

Since the regressions seem to be unrelated, we can merge this. Thanks!

@jedel1043 jedel1043 requested a review from a team July 9, 2023 18:38
@jedel1043 jedel1043 added this to the v0.18.0 milestone Jul 9, 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! :)

@HalidOdat HalidOdat added this pull request to the merge queue Jul 9, 2023
@dirkdev98
Copy link
Contributor Author

Thanks for the reviews ;)

Merged via the queue into boa-dev:main with commit fe9c8d9 Jul 9, 2023
9 checks passed
@dirkdev98 dirkdev98 deleted the fix-array-unscopables branch July 9, 2023 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
builtins PRs and Issues related to builtins/intrinsics enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants