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.plainDateTime #3037

Merged
merged 7 commits into from
Jul 16, 2021

Conversation

jugglinmike
Copy link
Contributor

In service of gh-3002

/cc @ptomato @Ms2ger

Ensure the error is thrown due to the invocation of the provided method.
Add a separate test to verify how the method is invoked.
The observable interactions with the "timeZone" parameter are verified
by another test which is named for that purpose.
This test's title suggests that it was intended to verify the behavior
when the "calendar" parameter was undefined. The expected behavior in
that case depends on the presence of a builtin calendar named
"undefined." Test262 cannot definitively assert the presence or absence
of such a calendar.

In contrast to the title, the test body actually uses the calendar name
"japanese."  Test262 cannot definitively assert the presence or absence
of such a calendar.
@@ -0,0 +1,46 @@
// Copyright (C) 2020 Igalia, S.L. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, originates here: tc39/proposal-temporal@60211b2

@@ -1,39 +0,0 @@
// Copyright (C) 2020 Igalia, S.L. All rights reserved.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to self: this was initially testing the case where Calendar.from was undefined:

Object.defineProperty(Temporal.Calendar, "from", {
  get() {
    actual.push("get Temporal.Calendar.from");
    return undefined;
  },
});

Need to check what happened here.

@rwaldron rwaldron merged commit 162e8be into main Jul 16, 2021
@rwaldron rwaldron deleted the bocoup/temporal-now-plain-date-time branch July 16, 2021 13:25
@Ms2ger
Copy link
Contributor

Ms2ger commented Jul 16, 2021

I would appreciate it if you didn't merge Termporal tests over my outstanding comments.

@rwaldron
Copy link
Contributor

Sorry, I don't remember seeing that comment. I reviewed all of the files, spent about an hour cross checking them with the original sources in https://github.com/tc39/proposal-temporal/tree/main/polyfill/test/Now/plainDateTime and when I was satisfied, I approved and merged. I'm sure it's possible that in the time between my review and then my review of the proposal's tests, that your comment came in, but my view didn't update for any number of unknowable reasons. Anyway, please consider it nothing more than an accident.

@rwaldron
Copy link
Contributor

I am curious though... how would you have liked me to handle a comment that starts with "Note to self:"?

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