mint-metrics / mojito-js-delivery

🧪 Source-controlled JS split testing framework for building and launching A/B tests.
https://mojito.mx/docs/js-delivery-intro
Other
16 stars 30 forks source link

Fixed an issue of utils.watchElement function #51

Closed allmywant closed 4 years ago

allmywant commented 4 years ago

I found this issue while I was working on tfes tests: If we call watchElement once, it works as expected. If we call watchElement more than one times, then only the callback of the first call will be fired.

kingo55 commented 4 years ago

Thanks @allmywant nice spot! Are those tests dependent on us merging this PR here in upstream?

Adding docs reference here for when we can review this in the morning: https://mojito.mx/docs/js-delivery-utilities#mojitoutilswatchelement

kingo55 commented 4 years ago

When I run this test locally, I'm getting an issue:

  1) Mojito utils functions
       WatchElement changes:
     Timeout of 2000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

It looks like done() is not being called. When I review the test, we are checking that both elements have changed within each watchElement() callback function:

                if (testDivChanged&&testDiv2Changed)
                {
                    done();
                }

Shouldn't we only check that the watched element only has updated in each block?:

            Mojito.utils.watchElement('.watch-changes', function (mutationsList)
            {
                testDivChanged = true;
                if (testDivChanged)
                {
                    done();
                }
            });

            Mojito.utils.watchElement('.watch-changes2', function (mutationsList)
            {
                testDiv2Changed = true;
                if (testDiv2Changed)
                {
                    done();
                }
            });

When I change the unit test to this, it works here.

dapperdrop commented 4 years ago

Just so I understand what the bug was: watchElement() was creating one instance of MutationObserver as domWatcher. On second and subsequent calls of watchElement() this original instance of domWatcher is called, rather than a new instance.

allmywant commented 4 years ago

@kingo55 I have workaround for those tests so no hurry on this. The unit test works fine on my end. The reason I wrote the check like this is I want to ensure both callback are called as expected.

if (testDivChanged&&testDiv2Changed)
{
       done();
 }

btw: did you run gulp build first before running gulp test ?

kingo55 commented 4 years ago

Ah yes, of course. Need to remember to run build first.

Perhaps we should have build run as a precursor to test. Though that's a topic for another PR/Issue...