Add tests for Temporal.Calendar.p.date(Add|FromFields)

[FrankYFTang]

@jugglinmike @ryzokuken @Ms2ger

[rwaldron]

If the methods are called "dateAdd" and "dateFromFields", then they belong in directories called "dateAdd" and "dateFromFields" (and so on).

[FrankYFTang]

If the methods are called "dateAdd" and "dateFromFields", then they belong in directories called "dateAdd" and "dateFromFields" (and so on).

ok, will do

[FrankYFTang]

directory name fixed, PTAL

[ptomato]

Some of these aren't necessarily "out of range" but "invalid" or "not agreeing with the month"

[FrankYFTang]

The spec text does not define those term. and you can view "m1" as "out of the range" of the range of string "M01" ... "M12", right? And while the month is specified, and the monthCode does not agree with it, it is a form of out of the "agreed range", right?

[FrankYFTang]

the point is all of them, as specified by the spec, throw RangeError. So my comment of "out of range" is reflecting that RangeError.

[jugglinmike]

These tests cover a lot of ground! Because assertions in Test262 operate by throwing exceptions, a given test can only fail due to one violated assertion. That's why we generally try to limit the number of assertions placed in a single file. Doing so makes the test suite more valuable to implementers since it gives them a more precise picture when something is incorrect.

That said, we certainly accept tests with more than one assertion. Choosing a good number of assertions per test is a bit subjective (and contextual), so we're not too strict about that.

I think the files named simple.js push the limit because it's not clear what makes each of their assertions distinctive. The way the code is formatted makes me think that there is some conceptual structure (perhaps involving boundary conditions of the various components of a Temporal date), but it's not evident to me as a reviewer.

Documenting that structure in the code itself will help others understand what functionality you intended to cover and potentially identify any areas you overlooked. It will also allow you to write more meaningful test descriptions and file names (the tests' current descriptions are somewhat vague because they are simply the name of the method under test, and the name simple.js is also a little generic).

Separately, every time an algorithm uses the "?" shorthand, it's describing an observable branch in the flow of control. We don't generally test every possible abrupt completion from each of these places, but we do try to verify at least one. Could you add tests for these?

[FrankYFTang]

These tests cover a lot of ground! Because assertions in Test262 operate by throwing exceptions, a given test can only fail due to one violated assertion. That's why we generally try to limit the number of assertions placed in a single file. Doing so makes the test suite more valuable to implementers since it gives them a more precise picture when something is incorrect.

What you stated perfectly make sense. I am not a good test writer. I am just lazy and dump all the test line into a big giant file while I prototyped instead of organize them well since it is very tiresome to create more test files than a giant one (and I have make all of them passed anyway). But I totally agree break them down into a lot of small files is easier to maintian in the long run.

I think the files named simple.js push the limit
I am not very created about naming them I know.

I literally randomly find some condition in the spec text and wrote some tests. I believe temporal proposal will copy a much completed set of test here later right?

Separately, every time an algorithm uses the "?" shorthand, it's describing an observable branch in the flow of control. We don't generally test every possible abrupt completion from each of these places, but we do try to verify at least one. Could you add tests for these?

I am not sure what you are asking. For example, let's say we have

Temporal.Calendar.prototype.dateAdd
4. Set date to ? ToTemporalDate(date).

What are you asking me to do with that "?" ?

[jugglinmike]

But I totally agree break them down into a lot of small files is easier to maintian in the long run.

Great, thank you!

I think the files named simple.js push the limit
I am not very created about naming them I know.

I literally randomly find some condition in the spec text and wrote some tests. I believe temporal proposal will copy a much completed set of test here later right?

Good question! As documented in the "Testing the Temporal" tracking issue, no one else has publicly expressed an intent to write these tests, so you don't have to worry about interfering with anyone else's work.

As for the tests in the Temporal proposal repository: those can be included here in Test262 since their licences are compatible. That means you don't have to write those tests yourself--you can copy them in. In my experience, they sometimes need a little bit of modification (e.g. to extend coverage or to satisfy Test262's formatting requirements), but that's fine, too. Just be sure to preserve the original license text at the top of each test.

(By the way, I've updated the list to reflect the additional testing work you began yesterday. To be even more careful about duplicating effort, you can leave a comment there before starting to write tests.)

Separately, every time an algorithm uses the "?" shorthand, it's describing an observable branch in the flow of control. We don't generally test every possible abrupt completion from each of these places, but we do try to verify at least one. Could you add tests for these?

I am not sure what you are asking. For example, let's say we have

Temporal.Calendar.prototype.dateAdd
4. Set date to ? ToTemporalDate(date).

What are you asking me to do with that "?" ?

That particular application of ? means we should have at least one test which asserts that Temporal.Calendar.prototype.dateAdd throws an exception when the ToTemporalDate abstract operation returns an abrupt completion. Hopefully an example will make this a little easier to understand:

var cal = new Temporal.Calendar("iso8601");
var duration = new Temporal.Duration(1);

assert.throws(RangeError, function() {
  cal.dateAdd("invalid date", duration);
});

A more precise version of that same test would simultaneously give input that would produce an different exception later on in the algorithm. That allows the test to also verify that the step you've highlighted occurs in the correct sequence.

 var cal = new Temporal.Calendar("iso8601");
-var duration = new Temporal.Duration(1);
+// Because the following object lacks any of the properties that describe a
+// Temporal Duration, passing it to the `dateAdd` method will normally produce
+// a `TypeError`. This test verifies that the `RangeError` from the invalid
+// date takes precedence.
+var invalidDuration = {};
 
 assert.throws(RangeError, function() {
-  cal.dateAdd("invalid date", duration);
+  cal.dateAdd("invalid date", invalidDuration);
 });

I suggested that we should have "at least one test" for that ? because there are a lot of ways to make ToTemporalDate return an abrupt completion, but we don't necessarily have to verify them all in the tests for Temporal.Calendar.prototype.dateAdd.

(In case you're curious: we will eventually need to exhaustively test the ToTemporalDate abstract operation, but for organizational purposes, we prefer to use the most direct/simple method available. In this case, it will probably be Temporal.PlainDate.from.)

[FrankYFTang]

What are you asking me to do with that "?" ?

So... I try to code that for dateAdd and can do so for step 1 -7, but I don't think I can wrote one for the following two

8. Let result be ? AddISODate(date.[[ISOYear]], date.[[ISOMonth]], date.[[ISODay]], duration.[[Years]], duration.[[Months]], duration.[[Weeks]], duration.[[Days]], overflow).
9. Return ? CreateTemporalDate(result.[[Year]], result.[[Month]], result.[[Day]], calendar).

9 is easier to explain why I have problem to write one- all the value passing in as arguments to CreateTemporalDate are valid by the previous steps so I do not belive I can craft a test to make any of the values cause the CreateTemporalDate to throw here.
Similar issue to AddISODate if you look into the internal. The AddISODate AO itself may throw only if BalanceISOYearMonth or BalanceISODate is buggy. IF both of them are doing the as what the spec intend to do (I am not sure the spec does that though) then AddISODate should not throw even if the overflow is set to "reject".

Please advise what I need to do to test for those kind of cases?
I will do the same for dateFromFields soon. Just upload those for dateAdd to get your feedback sooner.

[FrankYFTang]

ok, I added some more tests for dateFromFields, but still do not know how to force

7. Return ? CreateTemporalDate(result.[[Year]], result.[[Month]], result.[[Day]], calendar).

to throw

[jugglinmike]

Please advise what I need to do to test for those kind of cases?

That's a great insight! And not just for writing tests. Understanding when (and why) some invocations will never fail is relevant for anyone implementing the proposal. It means that the branch introduced by the ? is superfluous. Ideally, the proposal should document that these particular invocations will never return an abrupt completion. ECMA262 has a different notation to communicate that: instead of ?, it uses !.

(Please forgive me if you knew this already; I'm not familiar with your experience on this topic, and I don't want to assume too much. Plus, others reading this may benefit from a little more explanation.)

When I've noticed similar cases in the Temporal proposal, I've submitted patches to make the improvement (here's an example). Would you like to do something similar for the cases you found? Or would you like me to do that?

[FrankYFTang]

When I've noticed similar cases in the Temporal proposal, I've submitted patches to make the improvement (here's an example). Would you like to do something similar for the cases you found? Or would you like me to do that?

I am not too confident about my understanding of these notation and the topic of "abrupt completion" so I would prefer you file the issue for now. I may catch up and file some more later. But I have already filed way too many issues on Temporal now and I am pretty sure your voice will be more refreshing in the issues list than me now :)

[jugglinmike]

I am not too confident about my understanding of these notation and the topic of "abrupt completion" so I would prefer you file the issue for now. I may catch up and file some more later. But I have already filed way too many issues on Temporal now and I am pretty sure your voice will be more refreshing in the issues list than me now :)

I attempted to write these issues, and in the process, I found that both of the Abstract Operations may return an abrupt completion in this context. In other words: we don't have to change the proposal, but we do need to write automated tests. (By the way, trying to write normative spec/proposal changes is an effective way to verify theories like this!)

9 is easier to explain why I have problem to write one- all the value passing in as arguments to CreateTemporalDate are valid by the previous steps so I do not belive I can craft a test to make any of the values cause the CreateTemporalDate to throw here.

The first three arguments will always be valid integers, but they may not describe a date that can be expressed with Temporal. If the input date is on the boundary of the Temporal.Instant limit, and we attempt to add a duration to that, then the result may be too far in the future. In that case, the invocation of ISODateTimeWithinLimits inside CreateTemporalDate would throw a RangeError. For example:

cal.dateAdd('+275760-09-13', new Temporal.Duration(0, 0, 0, 1));

An even better test would describe the boundary more precisely by adding a duration of a single nanosecond to the final date that can be expressed with Temporal. Could you add a test like that?

Similar issue to AddISODate if you look into the internal. The AddISODate AO itself may throw only if BalanceISOYearMonth or BalanceISODate is buggy. IF both of them are doing the as what the spec intend to do (I am not sure the spec does that though) then AddISODate should not throw even if the overflow is set to "reject".

Consider RegulateISODate. The first invocation may return an abrupt completion if the "intermediate" month and year don't support the given day. That will happen when adding a duration of "1 month" to the date 2021-01-29 because the February 29, 2021 is not a valid date:

calendar.dateAdd('2021-01-29', new Temporal.Duration(0, 1), {overflow: 'reject'});

Could you add a test for that?

ok, I added some more tests for dateFromFields, but still do not know how to force

7. Return ? CreateTemporalDate(result.[[Year]], result.[[Month]], result.[[Day]], calendar).

to throw

We can observe this using the boundary condition like above:

cal.dateFromFields({year: 275760, month: 9, day: 14});

Or we can pass a value that "overflows" and configure RegulateISODate to "reject" such values:

cal.dateFromFields({year: 2021, month: 1, day: 32}, {overflow: 'reject'});

We don't necessarily have to use both techniques, but would you mind adding a test that uses one of them?

[rwaldron]

A number of issues that have come up in this PR are repeated across all of the Temporal PRs. Wherever possible, it would be great if you could preemptively (preemptive to the review they will subsequently receive) update all of them wherever/whenever possible.

[FrankYFTang]

A number of issues that have come up in this PR are repeated across all of the Temporal PRs. Wherever possible, it would be great if you could preemptively (preemptive to the review they will subsequently receive) update all of them wherever/whenever possible.

sure. sorry I took the whole week off last week for a personal issue. I will try to update all the test262 PR I proposed the next 5 days. Sorry for the delay

[rwaldron]

@FrankYFTang thanks for checking in. I hope everything is ok. Please don't hesitate to say "let's pause" on these PRs if you need time or a break—there's no rush at the moment <3

[FrankYFTang]

We don't necessarily have to use both techniques, but would you mind adding a test that uses one of them?

I got your point and could work on that but I feel this PR is too large to be manage already. Could we do that in a separate PR? I am not claiming adding test to test ALL aspect of the Temporal.Calendar.dateAdd method here. I understand it is shard to figure out we have test to cover necessary aspect of a method once some tests for that method is landed but I am worry if we have to have a complete test to cover a method before we can land any test for that method to test262, it could be hard to move thing forward.

[FrankYFTang]

We don't necessarily have to use both techniques, but would you mind adding a test that uses one of them?

I got your point and could work on that but I feel this PR is too large to be manage already. Could we do that in a separate PR? I am not claiming adding test to test ALL aspect of the Temporal.Calendar.dateAdd method here. I understand it is shard to figure out we have test to cover necessary aspect of a method once some tests for that method is landed but I am worry if we have to have a complete test to cover a method before we can land any test for that method to test262, it could be hard to move thing forward.

Feel free to file a TODO issue and assigment me if you. I love to do that but I cannot assign an issue to myself in test262

[FrankYFTang]

PTAL

1. I split the test into smaller files and change description
2. I use the test harness landed last week
3. I address some (but not all) of the test of exception throwing.

[rwaldron]

I'm going to accept this so that progress can move forward, however we will need to audit coverage for these APIs in follow ups.