Temporal: Reorganize calendar and wrong-type tests #4415

brageh01 · 2025-03-04T11:32:48Z

Changes made to:

PlainDate
PlainYearMonth
ZonedDateTime
PlainDateTime
PlainMonthDay

Merged calendar-number.js and calendar-wrong-type.js, due to no special behaviour of number to be tested.

Extracted string tests to calendar-iso-string.js and renamed the file to calendar-invalid-iso-string.js, due to the contents of the file initially testing invalid ISO strings.

Questions:

Are change to be made for the argument-propertybag-calendar-number.js and argument-propertybag-calendar-wrong-type.js files as well?
However, I notice that the argument-propertybag-calendar-iso-string.js actually tests for valid and not invalid ISO strings here. Perhaps create a invalid-iso-string.js?

Is the "merge-and-extract» also relevant for relativeto-propertybag-calendar-number.js and relativeto-propertybag-calendar-wrong-type.js?

sffc · 2025-03-05T19:41:32Z

Issue: #3873

sffc · 2025-03-05T19:48:49Z

About the questions: my sense is that the propertybag tests are probably also in scope to be refactored here in a similar way.

…ring test into a separate file for relativeto-propertybag

…type-tests

…de; missing newline

ptomato

Thanks! It looks like this is on the right track. There may be some tests still left to do. For example, there should be relativeto-propertybag-... tests in Duration/prototype/round/ and Duration/compare/ as well, and I think there are a lot more argument-propertybag-... tests; there should be some for every API entry point that takes a Temporal object, for example withPlainTime methods.

Feel free to use a script to automate a first draft of changes if there are a lot of files to do.

To answer your questions:

Are change to be made for the argument-propertybag-calendar-number.js and argument-propertybag-calendar-wrong-type.js files as well?

Yes please.

However, I notice that the argument-propertybag-calendar-iso-string.js actually tests for valid and not invalid ISO strings here. Perhaps create a invalid-iso-string.js?

That sounds correct.

Is the "merge-and-extract» also relevant for relativeto-propertybag-calendar-number.js and relativeto-propertybag-calendar-wrong-type.js?

Yes.

ptomato · 2025-03-11T23:35:41Z

test/built-ins/Temporal/ZonedDateTime/from/argument-propertybag-calendar-wrong-type.js

@@ -9,20 +9,23 @@ description: >
 features: [BigInt, Symbol, Temporal]
 ---*/

+const timeZone = "UTC";


I see you found a pre-existing error in this test, we forgot the time zone. Thanks!

(Well, technically it's not an error; in step 4.b-c of ToTemporalZonedDateTime the check for a faulty calendar occurs before the check for a missing time zone. So the test was still correct, but it was testing something different than I expected.)

Normally I would suggest that we should make a separate test file to ensure that the "faulty calendar" exception is thrown before the "missing time zone" exception, but in this case I think no further action is needed. We generally don't test that sort of thing anyway currently. (See also #2746 for a draft of how we might test it in the future.)

ptomato · 2025-03-12T00:43:04Z

One more thing I forgot... please take a look at the indentation. We don't generally enforce particular coding style or indentation choices, but there are some files in this draft where the indentation is a mix of 2 and 4 spaces and we should try to avoid that.

…type-tests

…ral types

ptomato · 2025-04-11T20:44:20Z

@brageh01 Looks like there's some new stuff on here, thanks! When it's ready for another review, please let me know either on Matrix chat or by clicking the "re-request review" button.

…type-tests

…dentation

ptomato

@brageh01 Thank you for all the work! It looks nearly complete now. Just some minor comments.

ptomato · 2025-04-17T22:26:44Z

test/built-ins/Temporal/Duration/prototype/total/relativeto-propertybag-invalid-iso-string.js

-// Copyright (C) 2022 Igalia, S.L. All rights reserved.
+// Copyright (C) 2025 Igalia, S.L. All rights reserved.


No need to update copyright years in existing files (unless you're really rewriting a substantial portion of the file.) This comment applies to several files.

I believe updating the copyright year for this file is correct. Instead of deleting the previous calendar-number.js I renamed the file to an invalid-iso-string.js, but it is initially a new file with different functionality.

Looks like I’ve forgotten the «calendar» tag as well. I will fix it.

ptomato · 2025-04-17T22:34:11Z

test/built-ins/Temporal/Duration/prototype/total/relativeto-propertybag-invalid-iso-string.js

-  -19970327,
-  1234567890,
+const invalidStrings = [
+  ["", "empty string"],


Optional: you could consider adding more cases in tests such as this one. For example: strings that are neither ISO strings nor valid calendar identifiers. (If you decide this is out of scope for this PR, that would be fine, but I'd appreciate if you could open a follow up issue on GitHub in that case.)

I now see that I have not done the merge and extract for round in the same file location which I will do. In regards of your suggestion of adding more cases in tests "such as this one", do you imply only for the relativeto-propertybag files, or all of the calendar-invalid-iso-string files in general?

I believe it will be more transparent to create a new issue for this, but I can both create a new issue and start a PR after knowing more specificly which files that are relevant.

I meant for the calendar-invalid-iso-string files in general. Every file where the only test case is ["", "empty string"]. I agree, it's fine to create a new issue.

I found an already open issue that covers this topic, I think: #3896

ptomato · 2025-04-17T22:35:16Z

test/built-ins/Temporal/PlainDate/calendar-invalid-iso-string.js

@@ -0,0 +1,21 @@
+// Copyright (C) 2025 Igalia, S.L. All rights reserved.


I think it's OK to put your own copyright here (yourself, or University of Bergen, whatever applies in this case.) Although the test is similar to one that Igalia wrote, but it's a new test.

ptomato · 2025-04-17T22:37:26Z

test/built-ins/Temporal/PlainDate/compare/argument-propertybag-calendar-wrong-type.js

For this test and other files like it, we should now be able to merge primitiveTests and typeErrorTests into one array of tests that we can just call typeErrorTests or wrongTypeTests or even just tests, and merge the two loops into one. We no longer have to deal with some entries giving a RangeError.

ptomato · 2025-04-17T22:44:45Z

test/built-ins/Temporal/PlainDateTime/prototype/withCalendar/calendar-invalid-iso-string.js

+
+const invalidStrings = [
+  ["", "empty string"],
+  ["1997-12-04[u-ca=iso8601]", "ISO string with calendar annotation"],


I think this line should not be here. According to my reading of the proposal, it's valid to pass an ISO string to this method.

As suggested: tc39#4415 (comment)

…type-tests

…eTime As requested in PR comment: tc39#4415 (comment)

…type-tests

…ateTime

…onthDay

…earMonth

…type-tests

…ateTime

…on/prototype round and total

ptomato

Thank you for all the work on this pull request! This looks all correct.

Reorganize calendar and wrong-type tests

6cc037e

brageh01 requested a review from a team as a code owner March 4, 2025 11:32

brageh01 changed the title ~~Reorganize calendar and wrong-type tests~~ Temporal: Reorganize calendar and wrong-type tests Mar 5, 2025

Brage Hogstad and others added 9 commits March 11, 2025 13:50

Merged number.js tests into wrong-type.js tests, and extracted the st…

1c79535

…ring test into a separate file for relativeto-propertybag

Merged and extracted argument-* tests in PlainDate/from

e39813f

Merge branch 'main' into merge-and-extract-calendar-number-and-wrong-…

cd49862

…type-tests

added newline to argument-*-invalid-iso-string.js, due to an error-co…

b3d0e46

…de; missing newline

Merged and extracted argument-* tests for PlainYearMonth

ef15474

Merge and extract argument-* tests in ZonedDateTime/from

9f83e49

Merge and extract argument-* tests in PlainDateTime/from

9dfd5ad

Merge and extract argument-* tests in PlainMonthDay/from

60112c0

Corrected a type-error which occured in creation of a file

b0e8885

brageh01 marked this pull request as draft March 11, 2025 14:07

Corrected a type-error which occured when renaming a file

2e56fc1

ptomato reviewed Mar 11, 2025

View reviewed changes

brageh01 and others added 4 commits April 7, 2025 13:54

Merge branch 'main' into merge-and-extract-calendar-number-and-wrong-…

47df386

…type-tests

Merge and extract operation for files found in /compare for the tempo…

8d23556

…ral types

Fixed syntax issues regarding /compare files for some temporal types

3ca6287

Merge and extract for tests in PlainDate/prototype

6e4e4b9

brageh01 force-pushed the merge-and-extract-calendar-number-and-wrong-type-tests branch from 6eaedd0 to 6e4e4b9 Compare April 16, 2025 11:44

Brage Hogstad added 2 commits April 16, 2025 13:44

Merge and extract for PlainDateTime/prototype equals and since

588a588

Merge and extract for PlainDateTime/prototype until

3b39021

brageh01 force-pushed the merge-and-extract-calendar-number-and-wrong-type-tests branch from 6aa3d10 to 3b39021 Compare April 16, 2025 12:08

Merge and extract for PlainDateTime/prototype withCalendar

2d6491a

brageh01 force-pushed the merge-and-extract-calendar-number-and-wrong-type-tests branch from eb1a1da to 2d6491a Compare April 16, 2025 12:26

Merge branch 'main' into merge-and-extract-calendar-number-and-wrong-…

b1ad5ef

…type-tests

Brage Hogstad and others added 10 commits April 16, 2025 14:47

Merge and extract for PlainMonthDay/prototype equals

4529ce2

Merge and extract for PlainYearMonth/prototype equals

0bc5d9b

Fixed an error in the header for wrong-type.js in equals

d513a09

Merge and extract for PlainYearMonth/prototype since

25a6e8e

Merge and extract for PlainYearMonth/prototype until

614260d

Merge and extract for ZonedDateTime/prototype equals

e289ffa

Merge and extract for ZonedDateTime/prototype since

d668596

Merge and extract for ZonedDateTime/prototype until

53caaec

Merge and extract for ZonedDateTime/prototype withCalendar

a98d02f

Gone over altered files to correct some header mistakes and adjust in…

b6863aa

…dentation

brageh01 marked this pull request as ready for review April 17, 2025 13:36

ptomato requested changes Apr 17, 2025

View reviewed changes

Brage Hogstad and others added 13 commits April 23, 2025 12:08

Update copyright header in Temporal tests

6479588

As suggested: tc39#4415 (comment)

Merge branch 'main' into merge-and-extract-calendar-number-and-wrong-…

53ffbc6

…type-tests

Corrected mistake for invalid-iso-string for withCalendar in PlainDat…

7965082

…eTime As requested in PR comment: tc39#4415 (comment)

Added a missing 'calendar' for a invalid-iso-string.js file

b95e900

Merge and extract for Duration/prototype round

29cc2d7

Merged lists of types tested as calander ID into one array for PlainDate

1f79b75

Merge branch 'main' into merge-and-extract-calendar-number-and-wrong-…

06de7c6

…type-tests

Merged lists of types tested as calandar ID into one array for PlainD…

960c5f1

…ateTime

Merged lists of types tested as calandar ID into one array for PlainM…

c9e1b85

…onthDay

Merged lists of types tested as calendar ID into one array for PlainY…

9fc22ca

…earMonth

Merge branch 'main' into merge-and-extract-calendar-number-and-wrong-…

e84c9cc

…type-tests

Merged lists of types tested as calendar ID into one array for ZonedD…

f250834

…ateTime

Merged lists of types tested as calendar ID into one array for Durati…

7c39679

…on/prototype round and total

brageh01 requested a review from ptomato April 23, 2025 19:55

ptomato approved these changes Apr 23, 2025

View reviewed changes

ptomato merged commit 443f0d2 into tc39:main Apr 23, 2025
11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Temporal: Reorganize calendar and wrong-type tests #4415

Temporal: Reorganize calendar and wrong-type tests #4415

brageh01 commented Mar 4, 2025

sffc commented Mar 5, 2025

sffc commented Mar 5, 2025

ptomato left a comment

ptomato Mar 11, 2025

ptomato commented Mar 12, 2025

ptomato commented Apr 11, 2025

ptomato left a comment

ptomato Apr 17, 2025

brageh01 Apr 23, 2025

ptomato Apr 17, 2025

brageh01 Apr 23, 2025

ptomato Apr 23, 2025

ptomato Apr 23, 2025

ptomato Apr 17, 2025

ptomato Apr 17, 2025

ptomato Apr 17, 2025

ptomato left a comment

		// Copyright (C) 2022 Igalia, S.L. All rights reserved.
		// Copyright (C) 2025 Igalia, S.L. All rights reserved.

		@@ -0,0 +1,21 @@
		// Copyright (C) 2025 Igalia, S.L. All rights reserved.

Temporal: Reorganize calendar and wrong-type tests #4415

Temporal: Reorganize calendar and wrong-type tests #4415

Conversation

brageh01 commented Mar 4, 2025

sffc commented Mar 5, 2025

sffc commented Mar 5, 2025

ptomato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptomato commented Mar 12, 2025

ptomato commented Apr 11, 2025

ptomato left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ptomato left a comment

Choose a reason for hiding this comment