Skip to content

Start cleaning up staging/sm tests #4463

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

Open
wants to merge 142 commits into
base: main
Choose a base branch
from
Open

Start cleaning up staging/sm tests #4463

wants to merge 142 commits into from

Conversation

anba
Copy link
Contributor

@anba anba commented Apr 30, 2025

Changes:

  • Replace harness functions from harness/sm with standard test262 harness functions.
  • Remove now unused harness functions from harness/sm.
  • Both changes should help a bit for Deal with slow staging/sm tests #4402, because there's now less harness code to execute.
  • Avoid testing error messages, because error messages aren't standardised.
  • Remove unused files.
  • Remove noStrict flag where possible.
  • And many more clean-ups and fixes.

I've also identified a couple of tests which should be removed from test262, because they test non-standard behaviour:

  • Tests for Function.caller.
  • Date.parse with non-standard inputs.
  • Tests how accurate Math functions are.
  • If the ToString output for bound functions contains the function name.
  • Large RegExp tests.
  • Cycle detection tests.
  • Tests using SpiderMonkey shell functions like disassemble or timeout.

In a follow-up PR I can remove these tests, too.

@anba anba requested review from a team as code owners April 30, 2025 12:43
Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

Reviewed up to "Remove non262-extensions-shell.js"

@@ -10,7 +10,7 @@ function assertFalse(a) { assertEq(a, false) }
function assertTrue(a) { assertEq(a, true) }
function assertNotEq(found, not_expected) { assertEq(Object.is(found, not_expected), false) }
function assertIteratorResult(result, value, done) {
assertDeepEq(result.value, value);
assert.deepEqual(result.value, value);
Copy link
Contributor

Choose a reason for hiding this comment

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

@Ms2ger Does your importation script need to be updated to make this replacement as well?

Comment on lines 39 to 42
try
{
strict1(out);
throw "no error";
}
catch (e)
{
assert.sameValue(e instanceof TypeError, true, "expected TypeError, got " + e);
}
assert.sameValue(deepEqual(out.array, [1, 2, 3]), true);

// Internally, SpiderMonkey has two representations for arrays:
// fast-but-inflexible, and slow-but-flexible. Adding a non-index property
// to an array turns it into the latter. We should test on both kinds.
function addx(obj) {
obj.x = 5;
return obj;
}

function nonStrict2(out)
{
var a = out.array = addx(arr());
a.length = 2;
}

function strict2(out)
{
"use strict";
var a = out.array = addx(arr());
a.length = 2;
}

out.array = null;
nonStrict2(out);
assert.sameValue(deepEqual(out.array, addx([1, 2, 3])), true);

out.array = null;
try
{
strict2(out);
throw "no error";
}
catch (e)
{
assert.sameValue(e instanceof TypeError, true, "expected TypeError, got " + e);
}
assert.sameValue(deepEqual(out.array, addx([1, 2, 3])), true);
assert.throws(TypeError, function() {
strict1(out);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess maybe in the interest of max coverage of implementation strategies, we shouldn't necessarily remove this just because SpiderMonkey updated their implementation details? Making an array slow when adding a non-index property seems like a choice that other implementations might make.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The file tests that modifying length fails for an array with a non-configurable array element. If we want to have a test to cover that arrays support non-index properties, we shouldn't use this file for that. (And we probably already have coverage that arrays support non-index properties.)

The test itself was added in https://bugzilla.mozilla.org/show_bug.cgi?id=537873. And the "slow array" part became obsolete twelve years ago in https://bugzilla.mozilla.org/show_bug.cgi?id=827490.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consider keeping this while just removing the enableGeckoProfiling stubs? Or is there already explicitly coverage for this in the main tree?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test boils down to:

function* gen() {
  var x = yield 'hi';
  yield x;
  yield 'bye';
}

for (var unused of [ true, false ]) {
  g = gen();
  assert.sameValue(g.next().value, 'hi');
  assert.sameValue(g.next('gurgitating...').value, 'gurgitating...');
  for (var x of g)
    assert.sameValue(x, 'bye');
}

which is covered through various tests in "built-ins/GeneratorPrototype/next" and "language/statements/for-of".

Copy link
Contributor

@ptomato ptomato left a comment

Choose a reason for hiding this comment

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

Reviewed up to "Replace makeIterator with inline definitions"

@@ -22,10 +22,10 @@ function TestGeneratorObject() {

var iter = g();
assert.sameValue(Object.getPrototypeOf(iter), g.prototype);
assertTrue(iter instanceof g);
assert.sameValue(iter instanceof g, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd prefer assert(iter instanceof g); or better, assert(iter instanceof g, "insert appropriate debugging message") for these kinds of assertions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#4371 (comment) requested changing assert(expr) to assert.sameValue(expr, true).

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, too bad. Well, let's keep it as is then, I don't want to conflict with others' comments.

anba added 24 commits May 28, 2025 16:24
anba added 29 commits May 28, 2025 16:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants