intl402/NumberFormat/length.js is incorrect

[gsathya]

In intl402/NumberFormat/length.js,

verifyConfigurable calls isConfigurable which deletes the Intl.NumberFormat.length property.

verifyProperty tries to lookup this now deleted length property, using Object.prototype.hasOwnProperty which fails and throws.

This needs to be fixed.

Also, can we rename verifyConfigurable to verifyConfigurableAndDelete or something that shows that this is destructive?

This has happened before as well:
#1467

[rwaldron]

The problem isn't that verifyConfigurable needs to be renamed, it's simply that verifyConfigurable and verifyProperty must never be called on the same object for the same property in the same test. I'll fix this asap.

[ljharb]

Is there a reason the helpers couldn’t clean up after themselves, so as to avoid this footgun?

[rwaldron]

@ljharb they can, you just have to tell it to with the {restore: true} arg. In this case, I had meant to remove the old calls when I migrated the test to use verifyProperty(...) and it was just missed in review. Clerical error?

[ljharb]

Is there a reason restore true can’t be the default?

[leobalter]

@ljharb tests should be atomic, for that case, each test case should never produce side effects over other cases. The restore option is a workaround I consider a bad practice. Ideally you should rely on the original version for each test depending on it, and never restoring a thing to what we consider to be original. In the testing philosophy this might cause false positives.

In the case fixed at acf6de1, the problem was not about isolation but using the helpers to check the same thing twice.

I believe with the current granularity of Test262, we can perfectly live without ever having to use the restore option, and all the tests can always benefit from that.

I'd love to talk more about this if you're interested. Just let me know.

[gsathya]

Thank you for the quick fix, @rwaldron!

I agree with @leobalter and @rwaldron here about tests being atomic and we shouldn't have verifyProperty and verifyConfigurable testing the same thing twice. That was the bug in this case, not verifyConfigurable or verifyProperty in itself.

I suggested the renaming to verifyConfigurableAndDelete only because this makes it obvious to someone reviewing the code that we shouldn't be doing anything else after calling this method as it's deleted the property.

Is there anyway to codify the expectation that we shouldn't be calling the helpers on the same thing twice so that this doesn't happen in the future?