Testing best practice guides? waitFor is inoptimal

[lolmaus]

Hi!

I've tried integration-testing my component that relies on ember-in-viewport and found the tests to be flakey: randomly failing or passing. After pulling my hair as I spent hours debugging in wrong directions, I've realized that it's ember-in-viewport not playing along with await settled().

I've looked into ember-in-viewport own tests and found that they rely on waitFor(selector). I refactored my tests to use that and it seemed to resolve the flakiness.

But I find waitFor(selector) in tests to be a bad practice, for a number of reasons:

• Sometimes the element that should appear as a result of didEnterViewport/didExitViewport is unknown or irrelevant (out of the current test's scope).
• Sometimes didEnterViewport/didExitViewport don't cause anything to appear in DOM at all.
• Sometimes didEnterViewport/didExitViewport should remove something from the page, and there's no waitToDisappear helper.

Instead of relying on waitFor(selector), there should be a direct way of waiting for the didEnterViewport/didExitViewport to trigger.

In other words, there should be a way to rewrite this to be reliable without relying on some third-party element appearing in the DOM:

find(`[data-test-container]`).scrollTop = 9999999999;
await settled();

Maybe ember-in-viewport could provide its own helper to be awaited. Though I struggle to come up with specifics how it could work. My only idea is to rely on some token or custom identifier similar to how ember-lifeline works: await pollTaskFor('my-task-name').

CC @simonihmig

[lolmaus]

@snewcomer This issue is kinda big problem for us. Mind having a look?

[snewcomer]

@lolmaus Interesting ideas. On one hand, tests that invoke didEnterViewport usually result in some operation (whether sync/async) and I am able to assert the successful operation. So waitFor and waitUntil have been helpful whereas waiting for didEnterViewport may not necessarily work for, say lazy loading images or other async operations. But I agree that some cases may not be optimal without some custom test helper. Can you elaborate on a specific use case?

[lolmaus]

tests that invoke didEnterViewport usually result in some operation (whether sync/async) and I am able to assert the successful operation

That's not the case for me. :(

The problem is that the test resumes before didEnterViewport fires and initiates a waited-upon operation.

So in our tests we have to do this:

  When I visit /feed
  And pause for 2000 ms
  Then there should be 1 Item in the Feed

Items are fetched via store.query. The store.query operation is properly waited for, but the problem is that it is initiated by didEnterViewport.

didEnterViewport seems to be async (in a sense that it's time-delayed and not thread-blocking).

As a result, the test asserts for the number of items in the feed before didEnterViewport fires and initiates a waited-upon query.

PS Note that without the pause for 2000ms step, the test sometimes passes. But sometimes it fails. 😒

[snewcomer]

So something like this doesn't work?

        await waitUntil(() => {
            return findAll('[data-test-feed-item]').length === 2;
        })

Even { options: 3000 } as the second arg to waitUntil...

[lolmaus]

It does, but it's an ugly hack.

Instead of requiring an ad-hoc check to be crafted, unique for every case, the addon should provide a general way to wait for didEnterViewport.

Apparently, waitFor('.my-in-viewport-component') does not work because didEnterViewport sometimes fires asynchronously.

[snewcomer]

I think we need to first avoid referring to these as "ugly hacks". They are definitely not as long as the test is deterministic. These are simply ways of asserting the logic as a result of entering the viewport is in the DOM.

Now if we await for didEnterViewport, that may be a good idea. Would one have to necessarily return from that method for the await to properly work?

[lolmaus]

I think we need to first avoid referring to these as "ugly hacks".

I have explained in the top post why I consider waitFor ugly. waitUntil is slightly better since it is more versatile, but it's still suboptimal and a PITA to use.

There are situations when using a tip of a knife is a reasonable way of unscrewing a screw. I mean, when it's urgent and you have no screwdriver around. But "sometimes reasonable" does not prevent it from being an ugly trick that many people would like to avoid as much as possible.

---

   await waitUntil(() => {
       return findAll('[data-test-feed-item]').length === 2;
   })

They are definitely not as long as the test is deterministic.

Well, why do I have to hardcode that there are two elements on the page? Tomorrow the number may change, and you'll have to update more than one source of truth.

I could do length > 0, but what if store.query returns an empty array? Nothing will be rendered and the test will fail, and there is no way to do return findAll() because the number of items hasn't changed.

What if the component had already rendered, queried and displayed the elements, and I'm triggering didEnterViewport for the second time? Then waitUntil will resolve too early because there are already two elements on the page!

The waitUntil approach is definitely flawed because it's an attempt to fix a problem not where the problem is happening. It's like treating symptoms of a disease without curing the actual disease.

---

Now if we await for didEnterViewport, that may be a good idea.

In order to await for a method:

• you should be calling the method from your code;
• the method should initiate some routine and resolve the returned promise when the routine is finished.

Neither is true for didEnterViewport: it's a hook, triggered by the framework when it's already too late to start waiting.

I see two ways of solving the problem:

• using waitUntil somewhere in the addon internals;
• using registerWaiter.

Before thinking about the implementation, we must decide on an API to use in integration and acceptance tests.

Since there may be more than one in-viewport element on the page, we need to somehow reference which one we're waiting for. Something like:

import { waitForElementToEnterViewport, waitForElementToLeaveViewport } from 'ember-in-viewport/test-support';

await waitForElementToEnterViewport('foo');
await waitForElementToLeaveViewport('bar');

Note that those test helpers should NOT be accepting CSS selectors. Using selectors in acceptance tests is an extremely bad practice.

In a similar situation he ember-lifeline addon by @rwjblue is using a token shared between a component definition and a test:

export const POLL_TOKEN = 'zee token';

export default Component.extend({
  init() {
    this._super(...arguments);
    pollTask(this, 'updateTime', POLL_TOKEN);
  },
})

import { POLL_TOKEN } from 'my-app/components/update-time';

module('updating-time', function(hooks) {
    test('updating-time updates', async function(assert) {
      await pollTaskFor(POLL_TOKEN);
    });
});

@snewcomer, do you think we could do a similar thing with ember-in-viewport?

[snewcomer]

I think I have lost the motivation to put in the work here. If you would like to make a PR, I'd be happy to review and merge.

[lolmaus]

OK, but before I proceed to coding, I would like to agree upon the design and resolve some questions.

As I said above, I'm thinking about using custom tokens to reference components from tests.

A token could be set on component like this:

export const MY_JUMPY_COMPONENT_TOKEN = 'foo'; // Could also use `{}` or `Symbol()` as a unique token

export default class MyJumpyComponent extends Component {
  viewportToken = MY_JUMPY_COMPONENT_TOKEN;
}

Note: using the init() hook should not be required to set viewportToken.

Then it can be used in tests like this:

import { waitForElementToEnterViewport } from 'ember-in-viewport/test-support';
import { MY_JUMPY_COMPONENT_TOKEN } from 'my-app/components/my-jumpy-component';

module('foo', function(hooks) {
    test('bar', async function(assert) {
      await waitForElementToEnterViewport(MY_JUMPY_COMPONENT_TOKEN);
    });
});

The above is all the new API surface area.

---

Under the hood, I'm imagining the waitForElementToEnterViewport function to be implemented like this:

export async function waitForElementToEnterViewport(token) {
  await new Promise((resolve) => {
    SOME_KIND_OF_REGISTRY.waitForElementToEnterViewport('token', resolve);
  });
  
  return settled();
}

As you can see, it passes the token and a promise resolve callback to a registry.

The in-viewport mixin would trigger the callback on the registry during didEnterViewport. This could be added to the mixin:

didEnterViewport() {
  if (this.viewportToken !== undefined) {
    SOME_KIND_OF_REGISTRY.triggerEnterViewport(this.viewportToken);
  }
}

Note that it will require users to add super.didEnterViewport() into their hook implementations.

The registry could be implemented like this:

class SomeKindOfRegistry {
  enterViewportCallbacks = [];
  
  waitForElementToEnterViewport(token, resolve) {
    this.enterViewportCallbacks.addObject({token, resolve});
  }

  triggerEnterViewport(token) {
    this
      .enterViewportCallbacks
      .filterBy('token', token)
      .forEach(({resolve}) => resolve());
  }
}

Alternatively, the registry could be implemented as a simple array, instantiated and exported on the root level of a ES module. In that case all the logic for manipulating callbacks would need to go into the mixin and/or service.

Open questions:

1. Should the registry be implemented as a class or a simple array?
2. Should there be three arrays, one for each event type (enter, leave, scroll) or one array that includes callbacks for all types?
   In the latter case, array element would have a signature {eventType, token, resolve} rather than {token, resolve}.
3. Where should the registry be defined and where should it be instantiated? Could be a test hook initializer setupInViewport(hooks) or something else.
4. Should this new logic be stripped out of prod builds? If we want the registry to be defined in addon-test-support, then the app code should not be accessing it...
   There are various hacky ways of working around this contradiction:
   1. Manipulating Broccoli.
   2. Using a conditional import.
   3. Something else?
   4. The simplest way is to keep the registry in the prod codebase. It could be wrapped with a check for the test env. Slightly larger build size, no weird tricks.
5. How do we do checks for the test env? if (Ember.testing) is the simplest one, but accessing the Ember global (or importing 'ember') is ugly. :( Not sure if there's something better.

[lolmaus]

@snewcomer Bump.

[snewcomer]

Honestly I'm not sure if the juice is worth the squeeze. A lot of code to manage. I'll let you make that decision though. Otherwise not a whole lot to comment on other than thinking about what trade offs we are bringing onto this project.

[hoIIer]

@lolmaus did you ever come up with a good solution here? Trying to write acceptance test for a client-side pagination feature and basically can't get it to work due to nature of scrolling/waiting in the test etc

Although not ideal, I ended up finding that ember-concurrency's timeout() util seems to allow me to properly wait for the scroll... wonder if ember-test-helpers should adopt such a helper for cases like this?

    document
      .querySelector('[data-test-slots]')
      .scrollTop = 10000000;

    await timeout(50);

    slotCount = document
      .querySelectorAll('[class*="slot-item"]')
      .length;

    assert.equal(slotCount, 30);

https://github.com/machty/ember-concurrency/blob/master/addon/utils.js#L160

[lolmaus]

@erichonkanen Nah, I ended up doing this and I'm not happy about it:

    await waitFor('[data-test-loading]', {timeoutMessage: 'loading indicator should eventually appear'});

If there's a bug and the loading indicator would not appear, then the test would keep running until the testing suite times out.

await timeout(50);

This is also not good. One day it will take more than 50ms to do its thing, and your CI will crash with a false negative.

---

Extra waiting time and false negatives are not a big deal. They are a minor nuisance and not a sacrifice of guarantees.

So this feature request is not about fixing something that's broken in the addon. It's about getting rid of the sense of discomfort from using pessimal techniques in tests.

---

Here's another suggestion of how it could look like from the dev's perspective.

The addon could provide a scrollTop helper that accepts an HTMLElement and returns a promise:

import { find, findAll } from '@ember/test-helpers';
import { scrollTop } from 'ember-in-viewport/test-support';

// ...

assert.equal(findAll('[data-test-slot-item]').length, 10);
await scrollTop(find('[data-test-slots]'), 10000);
assert.equal(findAll('[data-test-slot-item]').length, 20);

From the addon maintainer's perspective, it's a lot less code to manage than with my suggestion to use the ember-lifeline's approach. Though the actual implementation details are yet to be figured out.

[hoIIer]

@lolmaus I like that approach! I wonder how hard to implement?

And definitely agree, I just had to get something working for the time being and so went with sub-optimal solution. A scrollTop helper would be really nice (possibly even better if it was part of ember-test-helpers)

[snewcomer]

@lolmaus Something I forgot to ask a while back. Do you think a token that is shared between the service and test land that awaits for didEnterViewport is a guarantee that all the items have been painted? (say loading/painting more images)

[snewcomer]

I am willing to explore this if you would like. However, I'm still curious if a token will tell us "done" state (that is asserting a list length of x items). Do you have any thoughts?

[lolmaus]

Do you think a token that is shared between the service and test land that awaits for didEnterViewport is a guarantee that all the items have been painted? (say loading/painting more images)

I'm still curious if a token will tell us "done" state (that is asserting a list length of x items)

Uhm, no idea. I guess it only looks at the root element of the component, if I understood the question correctly.

Also, the didEnterViewport hook only applies to Classic comonents. For Octane, it should be a modifier.

---

@snewcomer Are you sure you want to go the path of a global registry and tokens?

My last suggestion seems easier both for app developers and addon maintainers. Though I'm not sure how to exactly implement it.

[snewcomer]

Are you sure you want to go the path of a global registry and tokens?

I'm not sure. The trouble I see is some helper still may not tell you if the next list is actually painted on the screen, thus a dev's next assertion may be too early.

[lolmaus]

@snewcomer Sorry, I'm not following what you mean exactly.

Can you please elaborate with an example? 🙏