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

UI: Fix flaky time-related tests #19521

Merged
merged 18 commits into from
Mar 22, 2023
Merged

UI: Fix flaky time-related tests #19521

merged 18 commits into from
Mar 22, 2023

Conversation

hashishaw
Copy link
Contributor

@hashishaw hashishaw commented Mar 13, 2023

✨ Cleanup Date usage ✨

This has caused headaches during testing, because the value of new Date() constantly changes due to the nature of time. With these changes, we no longer call new Date() within the application, and instead call a utility timestamp.now(). Under the hood, timestamp.now just returns new Date(), but it can be easily stubbed within tests to make the concept of "now" within tests is consistent and much easier to test.

Another change made as part of this PR is replacing all instances of new Date() within tests (when used to generate a UID for engine and secret paths) with uuidv4 which was added as part of 1.13.

NOT part of these changes is swapping out Date.now throughout the codebase. This change is large enough as is, and we can revisit swapping that out at a later time

How to review this
Obviously a lot of files were changed, but most of the changes are fairly simple swaps. I recommend reviewing the code commit by commit, to see similar changes grouped together and larger refactors separated into their own commit. EDIT things got sort of messy at the end there, trying to sort through test failures. Most of the files have 2-3 lines changed, so hopefully it's not too bad 😬

TL;DR How to test components that use time

Let's say you have a component that receives some timestamp from an API, and then renders something different depending on if the timestamp is in the past, future, close, far away, etc. Within the component, we would use timestamp.now() for the current time for comparison rather than new Date().

Then, the test might look something like this:

import sinon from 'sinon';
import timestamp from 'core/utils/timestamp';
// ...other imports

module('Integration | Component | example-component', function (hooks) {
  setupRenderingTest(hooks);
  hooks.before(function() {
    // before all the module tests, we stub timestamp.now to return a static date of our choosing. 
    sinon.stub(timestamp, 'now').callsFake(() => new Date('2018-04-03T14:15:30'));
  })

  hooks.beforeEach(function() {
    // since timestamp.now is being called after we stubbed it, we know that the date will always return 2018-04-03T14:15:30.
    this.now = timestamp.now();
  })

  hooks.after(function() {
    // very important to reset the stub after these tests run, or it will still be stubbed for tests run afterward
    timestamp.now.restore();
  })

  test('it shows the right thing when date is in the future', function(assert) {
    // Now we can use a static date for which to compare to "now"
    this.set('apiDate', new Date('2019-01-01T12:30:00');
    await render(hbs`<ExampleComponent @apiDate={{this.apiDate}} />`)
    assert.dom(this.element).hasText('The API date is next year');
  });
});

If you want the value of now to change depending on the test, you could stub within the test. Note that if you do it this way but call timestamp.now in the beforeEach hook, the value of timestamp.now will not be stubbed at that point, and will give you the actual current time.

@hashishaw hashishaw added this to the 1.14 milestone Mar 13, 2023
@hashishaw hashishaw marked this pull request as ready for review March 13, 2023 22:08
await click('[data-test-previous-year]');
}
await click(find(`[data-test-calendar-month=${ARRAY_OF_MONTHS[customEndDate.getMonth()]}]`));
await click('[data-test-previous-year]');
Copy link
Contributor

Choose a reason for hiding this comment

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

so much nicer 🥲

Copy link
Contributor

@hellobontempo hellobontempo left a comment

Choose a reason for hiding this comment

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

Nice work on this so far! Thank you for tackling this cleanup work! Looking forward to less flaky tests 😍

UPGRADE_DATE,
'MMM d, yyyy'
)}. We added monthly breakdowns and mount level attribution starting in 1.10, so keep that in mind when looking at the data. Learn more here.`
`Warning Vault was upgraded to 1.10.1 on Aug 1, 2022. We added monthly breakdowns and mount level attribution starting in 1.10, so keep that in mind when looking at the data. Learn more here.`
);
});

test('Shows empty if license start date is current month', async function (assert) {
// TODO cmb update to reflect new behavior
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@hellobontempo is this TODO still relevant? The tests seem to pass but I'm not sure if this meant to check more things?

Copy link
Contributor

Choose a reason for hiding this comment

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

Oish yeah. I noticed this on my last passthrough and wondered the same thing 😅

Among the miscellany of other bugs, I don't think I ever got an answer from the backend on what the expected response in this case (or perhaps forgot to ask). Previously data wasn't available for a customer until the first completed month, but that should no longer be the case.

This test can be deleted! But a good reminder of something that should be taken into account for the latest backend work on activity log testing

await render(hbs`{{date-format this.timestampDate 'MMM d yyyy, h:mm:ss aaa' withTimeZone=true}}`);
assert.dom(this.element).hasTextContaining(`${zone}`);
await render(
hbs`<span data-test-formatted>{{date-format this.timestampDate 'MMM d yyyy, h:mm:ss aaa' withTimeZone=this.withTimezone}}</span>`
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Might have just been due to running this test locally, but I found that it was expecting "CST" and giving "CDT" (in tests run on main as well). Since the time just switched, I figured testing it this way (making sure there's the expected number of characters) ensures that daylight savings doesn't ruin this test in the future. Other suggestions welcome

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this makes sense!

Copy link
Contributor

@Monkeychip Monkeychip left a comment

Choose a reason for hiding this comment

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

So great to see these changes. Thank you for going after this. Quick question, do you think some kind of comment like you have in the PR description can be added to the pattern doc, with a link to this PR?

@hashishaw
Copy link
Contributor Author

So great to see these changes. Thank you for going after this. Quick question, do you think some kind of comment like you have in the PR description can be added to the pattern doc, with a link to this PR?

Absolutely! Great idea, thanks for the review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants