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

Clean-up contribution guidelines, dependencies, Test262, MSRV #1683

Merged
merged 3 commits into from Oct 23, 2021

Conversation

Razican
Copy link
Member

@Razican Razican commented Oct 22, 2021

Multiple small changes in this PR:

  • Updates all dependencies in Rust and Yarn, also cleans-up the test_ignore.txt file a bit, where we had legacy comments.
  • It uses the new flag in Cargo 1.56 to indicate that the minimum supported Rust version is, in fact, 1.56, which should help some compatibility diagnostics.
  • Updates the Test262 sub-module with the latest changes to the official test-suite.
  • It adds a clarification in the contribution guidelines to make users of the Boa tester know that they can get more complete output by using the "very, very, verbose" output mode.
  • It fixes a small documentation warning.

This commit updates all dependencies in Rust and Yarn, also cleans-up
the `test_ignore.txt` file a bit, where we had legacy comments.

It uses the new flag in Cargo 1.56 to indicate that the minimum supported
Rust version is, in fact, 1.56, which should help some compatibility
diagnostics.

It also updates the Test262 sub-module with the latest changes to the
official test-suite.

Finally, it adds a clarification in the contribution guidelines to
make users of the Boa tester know that they can get more complete
output by using the "very, very, verbose" output mode.
@Razican Razican added enhancement New feature or request dependencies Pull requests that update a dependency file documentation update documentation test Issues and PRs related to the tests. labels Oct 22, 2021
@Razican Razican added this to the v0.14.0 milestone Oct 22, 2021
@github-actions
Copy link

Test262 conformance changes:

Test result main count PR count difference
Total 86,438 86,464 +26
Passed 38,514 38,500 -14
Ignored 19,055 19,061 +6
Failed 28,869 28,903 +34
Panics 0 0 0
Conformance 44.56% 44.53% -0.03%
Broken tests (14):
test/built-ins/Reflect/getOwnPropertyDescriptor/return-from-data-descriptor.js [strict mode] (previously Passed)
test/built-ins/Reflect/getOwnPropertyDescriptor/return-from-data-descriptor.js (previously Passed)
test/built-ins/Reflect/getOwnPropertyDescriptor/symbol-property.js [strict mode] (previously Passed)
test/built-ins/Reflect/getOwnPropertyDescriptor/symbol-property.js (previously Passed)
test/built-ins/Reflect/getOwnPropertyDescriptor/return-from-accessor-descriptor.js [strict mode] (previously Passed)
test/built-ins/Reflect/getOwnPropertyDescriptor/return-from-accessor-descriptor.js (previously Passed)
test/built-ins/RegExp/named-groups/unicode-match.js [strict mode] (previously Passed)
test/built-ins/RegExp/named-groups/unicode-match.js (previously Passed)
test/language/computed-property-names/object/method/symbol.js [strict mode] (previously Passed)
test/language/computed-property-names/object/method/symbol.js (previously Passed)
test/language/computed-property-names/basics/symbol.js [strict mode] (previously Passed)
test/language/computed-property-names/basics/symbol.js (previously Passed)
test/language/expressions/object/cpn-obj-lit-computed-property-name-from-yield-expression.js [strict mode] (previously Passed)
test/language/expressions/object/cpn-obj-lit-computed-property-name-from-yield-expression.js (previously Passed)

Copy link
Member

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

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

LGTM, a bit concerned with the tests that stopped passing

@RageKnify
Copy link
Member

Looked into the failing tests, think I know what's wrong, here's the code of one of them:

var o1 = {
  p: 'foo'
};

var result = Reflect.getOwnPropertyDescriptor(o1, 'p');

assert.compareArray(
  Object.getOwnPropertyNames(result),
  ['value', 'writable', 'enumerable', 'configurable']
);
assert.sameValue(result.value, 'foo');
assert.sameValue(result.enumerable, true);
assert.sameValue(result.configurable, true);
assert.sameValue(result.writable, true);

After some debugging the problem seems to be that result gets overwritten after being assigned (changing var result to const result results in "TypeError": "Cannot mutate an immutable binding result"), I assume it is related to scoping, inside assert.compareArray there is also a var result that I'm guessing overwrites the previous value.

Maybe a problem with how vars are scoped in functions? Scoping is one area I'm not very familiar with so there might be someone better suited to fix this.

@raskad
Copy link
Member

raskad commented Oct 23, 2021

Yes this is definitely a scoping error. This issue describes the same error: #1663

@RageKnify
Copy link
Member

Yes this is definitely a scoping error. This issue describes the same error: #1663

Ok, I did think I'd seen it mentioned already, if we have an issue for it then I guess we can ignore the regression and let this PR through.

@Razican Razican merged commit a5c8570 into main Oct 23, 2021
@Razican Razican deleted the rust_version branch October 23, 2021 16:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file documentation update documentation 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.

None yet

3 participants