aurelia / testing

Simplifies the testing of UI components by providing an elegant, fluent interface for arranging test setups along with a number of runtime debug/test helpers.
MIT License
40 stars 27 forks source link

Testing a custom attributes' valueChanged() hook #70

Open MarcScheib opened 7 years ago

MarcScheib commented 7 years ago

I'm submitting a bug report

Please tell us about your environment:

Current behavior:

When testing a custom attribute with one bound value and valueChanged() method, the method is not called during the spec, although the value binding is working in e.g. an input field.

The valueChanged() is called during testing when changing the bound value.

Testing the logic of a custom attributes valueChanged() hook.

I am using the following code so far and binding is also working for the input, but not for the custom attribute:

import {bootstrap} from 'aurelia-bootstrapper';
import {Aurelia} from 'aurelia-framework';
import {StageComponent} from 'aurelia-testing';
import {DOM} from 'aurelia-pal';

import {BackgroundChanger} from '../../src/background-changer';

describe('BackgroundChange', () => {
  let component;
  let vm = { bg: 'blue' };

  beforeEach(() => {
    component = StageComponent
      .withResources('src/background-changer')
      .inView(`
<input id="bg-input" value.bind="bg">
<div id="bg" bg-change.bind="code"></div>
`)
      .boundTo(vm);
  });

  afterEach(() => {
    component.dispose();
  });

  it('changes the background on input change', done => {
    component.create(bootstrap)
      .then(() => {
        const inputEl = DOM.getElementById('bg-input');
        const divEl = DOM.getElementById('bg');

        expect(inputEl.value).toEqual(vm.bg);

        const newBg = 'red';

        inputEl.value = newBg;
        expect(inputEl.value).toEqual(newBg);
        inputEl.dispatchEvent(DOM.createCustomEvent('change'));
        expect(vm.bg).toEqual(newBg);
        expect(divEl.style.background).toEqual(newBg); // this one fails with expected blue to be red
      })
      .then(done);
  });
});
suneelv commented 7 years ago

@MarcScheib doing the assertion after a timeout might work as valueChanged callbacks are not called synchronously

setTimeout(() => {
  expect(divEl.style.background).toEqual(newBg); 
  done();
}, 50)
MarcScheib commented 7 years ago

Yes, that worked @suneelv . Thank you!

tomtomau commented 6 years ago

This affect me as well, and the above work around does not resolve it.

RomkeVdMeulen commented 4 years ago

I've run into a similar problem countless times in my own tests:

  1. Update some value on the VM
  2. Check the DOM: it hasn't been updated yet

The go-to workaround is to put step 2 inside a setTimeout or queue it as a macro task on Aurelia's TaskQueue. And that will solve the problem most of the time. But every once in a blue moon our CI server will report a test failing when it has succesfully run a thousand times before.

I'm getting very tired of it. I want to find a solution where the test will deterministically succeed.

I've been trying to figure out where the delay occurs, but I can't figure it out. From my understanding, each model update should synchronously trigger Aurelia's property observer, which puts a microtask on the queue to update linked bindings. The microtask queue is flushed with a DOM mutation observer which is a JS microtask. So by the time the setTimeout in my test, which is a macro task, resolves, the binding updates and the corresponding DOM updates should have run already, and if I test the DOM for the updated value that should succeed 100% of the time. And yet it doesn't. What am I missing?

Could someone explain it to me, please? @EisenbergEffect perhaps, or @fkleuver? Is there any way for me to delay the DOM check in my test so that the binding update is guaranteed to have been resolved?

tomtomau commented 4 years ago

@RomkeVdMeulen

Our solution has been to waitFor your assertion.


// Rather than
expect(divEl.style.background).toEqual(newBg); 

// waitFor instead, we have a helper function as the aurelia-testing is asserting against whether something is null rather than truthy/falsy
import { waitFor } from 'aurelia-testing';

export class WaitForBool {
  static async truthy(callback) {
    return await waitFor(() => callback() ? true : null);
  }

  static async falsy(callback) {
    return await waitFor(() => callback() ? null : false);
  }
}

// Then your actual assertion
await WaitForBool.truthy(() => divEl.style.background === newBg);
RomkeVdMeulen commented 4 years ago

@tomtomau Thanks for the suggestion. I'm already using that construct in some places. But we have tens of thousands of assertions in our tests, and I fear if we use waitFor for all of them it will slow down the runtime of our tests to the point that it becomes impractical to run them.

bigopon commented 4 years ago

@RomkeVdMeulen this is an issue with how our v1 setup the task queue. One trick that you can use to eliminate this problem forever is to reuse a single task queue instance in tests. There' many ways to do it, depends on your bootstrapping, one way is to wrap bootstrapper function with your one:

import {bootstrap as $bootstrap} from 'aurelia-bootstrapper';
import { TaskQueue } from 'aurelia-task-queue';

let taskQueue: TaskQueue;
export function bootstrap(aurelia: Aurelia) {
  if (taskQueue) {
    aurelia.container.registerInstance(TaskQueue, taskQueue);
  } else {
    taskQueue = aurelia.container.get(TaskQueue);
  }
  return $bootstrap(aurelia);
}

And later in your code, instead of setTimeout, you can inject the task queue from wherever, and do:

taskQueue.flushMicroTaskQueue();
RomkeVdMeulen commented 4 years ago

@bigopon Thanks for the tip! I'll try it out.

RomkeVdMeulen commented 4 years ago

Also does that mean this won't be an issue anymore in V2?

bigopon commented 4 years ago

Not at all, at least i believe so. For the above, make sure you flush all existing queues when disposing an app at the end of a test as well.

RomkeVdMeulen commented 4 years ago

@bigopon I tried your suggestion: I re-used one task queue throughout the test run, making sure to manually flush the micro queue before assertions and during tear down. It didn't seem to make a difference. I'm still seeing intermittent failures where the DOM wasn't updated in time for the assertion.

I'm also trying to understand why flushing the queue manually should make a difference. Isn't the micro task queue always flushed before the start of the next event loop? (I run my tasks in Chrome, so MutationObservers are supported)

RomkeVdMeulen commented 4 years ago

I've been trying to create a minimal reproducable case, but so far without much luck. I've looked into all kinds of contributing factors, like async/await or computed properties, but as far as I can see none of those should be able to create a delay beyond the current event loop.

Perhaps the issue isn't with timing, but with some state not being properly cleaned between test runs, like with issue #93. Because these errors are so infrequent, I'm having a devil of a time debugging them.

RomkeVdMeulen commented 4 years ago

It doesn't seem to be a case of stale view state. Let me give a simplified example of what I'm seeing:

export class MyVM {
  myProp = {
    id: Math.round(Math.random() * 1000),
    loading: false,
  };

  myMethod() {
    console.log("flipping boolean");
    this.myProp.loading = true;
  }
}
<button click.trigger="myMethod()" data-loading="${myProp.loading}" data-id="${myProp.id}">click here</button>
it("changes the boolean", async () => {
  // setup...
  console.log("click");
  myComponentTest.element.querySelector("button").click();
  console.log("wait...");
  await new Promise(res => setTimeout(res);
  console.log(myComponentTest.viewModel.myProp);
  console.log(myComponentTest.element.querySelector("button"));
});

And once in a dozen runs the output is:

LOG: click
LOG: flipping boolean
LOG: wait...
LOG: {id: 5231, loading: true}
LOG: <button click.trigger="myMethod()" data-loading="false" data-id="5231">click here</button>

Of course the real code has composition, more indirection, DI, computed properties, etc. but in essence this is what it boils down to.

bigopon commented 4 years ago

@RomkeVdMeulen here's a test that can be used to verify your repro. You can un-comment the method call in attached setInterval and decrease the interval to verify. It seems working fine to me https://gist.dumber.app/?gist=735a4a0bc868bde5cc7469bd11692562

Edit: i gave you a wrong example code though, sorry! It should have been

import { bootstrap as $bootstrap } from 'aurelia-bootstrapper';

let taskQueue;
export function bootstrap(configure) {
  const $configure = aurelia => {
    if (taskQueue) {
      aurelia.container.registerInstance(TaskQueue, taskQueue);
    } else {
      taskQueue = aurelia.container.get(TaskQueue);
    }
    return configure(aurelia);
  };
  return $bootstrap($configure);
}
RomkeVdMeulen commented 4 years ago

@bigopon You're right, the repro works fine. I've been trying to narrow down where the delay is coming from, but as you can imagine with the delay only occuring randomly among lots of test runs it's very slow going.

Thanks for your help. If I do find the problem I'll let you know. For now just consider this closed from my side.

lemoustachiste commented 4 years ago

Hijacking this thread to improve my general knowledge with this tool.

I was trying to verify that the click callback on a button was being called.

My initial implementation was with

const clickEvent = new Event('click');
myButtonElement.dispatchEvent(clickEvent);

That didn't trigger the click handler (called with click.delegate).

With the help of this comment https://github.com/aurelia/testing/issues/70#issuecomment-686568600 I realised I could use the HTMLButtonElement.click method, which indeed worked (even without having to delay the assertion).

My question is, why is the classic DOM Event method not being a trigger? Is this a preferred approach for any event or would there be cases where dispatchEvent would work?

Thanks

bigopon commented 4 years ago

@lemoustachiste there shouldn't be any issue with what you are trying to do. Can you help give a repro of what you want to do, based on the example here https://gist.dumber.app/?gist=650aecc59a32d08776692a4b5055b7c0