Tests and suggested implementation for Appointment Time are wrong

The tests for createAppointment calculate the target time as follows:

test('creates appointment 124 in the future', () => {
    const currentTime = Date.now();
    const expectedTime = currentTime + 10713600 * 1000;

    expect(createAppointment(124, currentTime)).toEqual(new Date(expectedTime));
});

This does not properly take into account shifts introduced by the beginning or end of Daylight Savings Time. The following solution from a student I mentored fails:

export function createAppointmentUTCDate(days, now = Date.now()) {
    const appointment = new Date(now);
    appointment.setDate(appointment.getDate() + days);
    return appointment;
}

That should work - it’s perfectly acceptable solution to the stated task. What’s worse, that code would have worked if they had filed it at a different time - namely if they had worked on this exercise not within 124 days of DST beginning or ending. Obviously, passing or failing tests should not depend on the time of year that exercises are worked on.

For the same reason, the suggested solution is wrong:

export function createAppointment(days, now = Date.now()) {
  return new Date(now + days * 24 * 3600 * 1000);
}

With the suggested solution, someone making an appointment for “at this time a week from now” may actually end up with an appointment an hour earlier or later, depending on their current date. I.e. at the time of this filing (25 July):

new Date(Date.now()) // Fri Jul 25 2025 15:13:49 GMT+0200 
new Date(Date.now() + 10713600 * 1000) // Wed Nov 26 2025 14:13:35 GMT+0100     <--- suddenly one hour earlier according to local time

These tests and the recommended implementation should be corrected. If that is not possible without invalidating existing solutions, at the very least, the exercise instructions should state that this is the intended behavior, as it is wildly unintuitive for an appointments API.

1 Like

Thank you for your detailed report. You are correct that we have a change to make here.

The correct implementation given the question is indeed:

const date = new Date(now);
date.setDate(date.getDate() + days);
  
return date;

This was missed during the initial exercise review, which I did at an ungodly hour.

Would you like to submit a PR? It would have the following changes:

  1. The examplar solution in .meta updated.
  2. The hints updated.
  3. Remove the test for 124 days. It doesn’t catch any extra issues.
  4. rolls over days, months, and years updated to use the same logic as uses the actual current time when it is not passed in.
  5. Your GitHub username added to "contributors" inside .meta/config.json

We don’t want setDate() and getDate() in the tests, to not “duplicate” the implementation inside the tests.

Do you want to avoid talking about the *UTC* variants of the getters and setters? This avoids the DST effects:

date.setUTCDate(date.getUTCDate() + days);
1 Like

It’s a good question to ask but I feel that “24 days in the future” both respecting DST and ignoring it completely are valid approaches unless we define (and limit) the implementation space.

I’d be okay with either approach but I’d like to collect some opinions.

For Glenn’s suggestion we should add to the instructions: “it is important to ignore all timezone related data such as daylight savings”.

The upside is that we can keep hard coded unix timestamps which will always be correct.

I would love to submit a PR - Exercism is an awesome site and I would love to make a humble contribution.

Unfortunately I’m not sure that I will be able to anytime soon. If someone else is available to step up, that might be quicker. Otherwise I’ll see what I can do.

Regarding this:

  1. Remove the test for 124 days. It doesn’t catch any extra issues.

I agree, but don’t think that will be enough. As long as it tests any projection into the future (which it will need to by the definition of the task), there is some window for failure. I think the tests need to be modified as:

  1. One test that checks that if no source date is passed, it uses the current time, but makes no adjustment to it.
  2. All other tests that do arithmetic need to pass in a source date.
1 Like

I feel that dealing with time zone quirks and DST is much closer to real world use case than ignoring it so I would vote for the hard(er) solution.

On the other hand, I can’t deny that I’m tempted by the simplicity of UTC, after all that’s the whole point of it.

1 Like

I agree, time zone quirks and DST should be respected.

I think we should keep in mind that a substantial number of people using this site to learn are only just beginning to learn how to program. These people will get their “feel” for what good code looks like from these exercises. An API that shifts appointments around arbitrarily is just poorly designed and should not be showcased here (even if it is “only” an exercise), unless we can come up with a reasonable rationale.

If we feel it’s so important to teach date arithmetics and UTC, we could always introduce another exercise (e.g. element half life or something similar based on fixed amount of times).

1 Like

That’s right.

Not a problem. Do you think time will free up in the next 2 weeks?

I can try, but I wouldn’t hold my breath. New job, new house and two little kids at home. I don’t have a lot of free time. :wink:

1 Like

I have taken a look at the tests - well, the ones for createAppointment. I don’t mean to be negative, but it seems to me that they all have issues.

test('uses the actual current time when it is not passed in', () => {
    const result = createAppointment(0);

    expect(Math.abs(Date.now() - result.getTime())).toBeLessThanOrEqual(
        // Maximum number of time zones difference
        27 * 60 * 60 * 1000,
    );
});

Will accept any implementation unless it is off by more than 27 hours.

test('uses the passed in current time', () => {
    const currentTime = Date.UTC(2000, 6, 16, 12, 0, 0, 0);
    const result = createAppointment(0, currentTime);

    expect(result.getFullYear()).toEqual(2000);
});

Will accept wrong implementations with an error of up to roughly half a year

I don’t understand why the tests are implemented in such a roundabout way?

It’s a first iteration so them having issues is not unexpected. Timezone handling was forgotten when this was implemented (and I didn’t catch it either). You’re allowed to be negative about it. Rookie mistake :grin:

The only thing we don’t want is this:

test('...', () => {

  const someDate = ...
  someDate.setDate(someDate.getDate() + days);

  const result = createAppointment(days)
  expect(result).toBe(someDate)
})

Because that copies the expected implementation.

Not really because you have the other tests, but I don’t understand why this test doesn’t do:

expect(result.getTime()).toBe(currentTime.getTime())

… as it shouldn’t change that at all.

2 Likes

I have opened a PR.

1 Like

Had forgotten to update hints and contributors. Fixed in the PR now.

As per your to-do list:

  1. rolls over days, months, and years updated to use the same logic as uses the actual current time when it is not passed in.

Can you elaborate what you mean by that? That test looks okay to me, as adds it just under two years to a fixed date in June. Hence, unlikely, to trigger a DST change. I’m not aware of any country changing their clocks in June, but I’m no expert, obviously.

If you want, I can add a manual guard with getTimezoneOffset, but it’s probably not needed here.

1 Like

Nah we can just leave it for now and edit is when we have to.