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 tests for Temporal.now.timezone #3011

Closed
wants to merge 4 commits into from

Conversation

jugglinmike
Copy link
Contributor

In service of gh-3002

/cc @ptomato @Ms2ger

Ms2ger
Ms2ger previously approved these changes Jun 11, 2021
features: [Reflect.construct, Temporal]
---*/

assert.sameValue(isConstructor(Temporal.now.timeZone), false, 'isConstructor(Math.min) must return false');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert.sameValue(isConstructor(Temporal.now.timeZone), false, 'isConstructor(Math.min) must return false');
assert.sameValue(isConstructor(Temporal.now.timeZone), false, 'isConstructor(Temporal.now.timeZone) must return false');

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops! Thanks

features: [Temporal]
---*/

assert(Object.isExtensible(Temporal.now.timeZone));
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
assert(Object.isExtensible(Temporal.now.timeZone));
assert.sameValue(Object.isExtensible(Temporal.now.timeZone), true);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This looks equivalent to me. Could you say a little about why we ought to use assert.sameValue instead of assert?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understood the former to be preferred in test262, but I may be misremembering. Happy to go either way.

Copy link
Contributor

Choose a reason for hiding this comment

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

@jugglinmike you're right, this is fine as is.

@Ms2ger assert() is discouraged in cases such as assert(foo === "something interesting"), which is better written as assert.sameValue(foo, "something interesting") because you'll get a more meaningful error message.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like I misremembered, thanks for the clarification. I couldn't find this discouragement in the documentation at https://github.com/tc39/test262/blob/main/CONTRIBUTING.md#test-environment - could you add it?

assert.sameValue(
Object.getPrototypeOf(Temporal.now.timeZone()),
Temporal.TimeZone.prototype
);
Copy link
Contributor

Choose a reason for hiding this comment

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

This test just tell me that those two things are the same thing, but explicitly that either of them is an instance of Temporal.TimeZone.

assert(Temporal.now.timeZone() instanceof Temporal.TimeZone)?

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 instanceof operator has a lot of additional semantics which would make this test less precise. For example, it would be satisfied if the return value was an instance of a subclass of Temporal.TimeZone. I believe a strict equality check is the most precise way to verify this behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could have both?

@rwaldron
Copy link
Contributor

Rebased locally and merged

@rwaldron rwaldron closed this Jun 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants