Multiple select list results in order - cont'd

Previously in the Multiple select list results in order of last selected on top thread, tests that should have earlier been there, were being added to the existing code.

One expect per test

Some of the tests have multiple expect checks that need to be broken up:

    it("gets the inner height of an element", function () {
        var innerHeight = scroll.innerHeight($(categories));
        expect(innerHeight).toBeGreaterThan(0);
        expect(innerHeight).toBeLessThan(categories.scrollHeight);
    });

These multiple expectations really need to be split up into separate tests:

    it("has a positive inner height", function () {
        var innerHeight = scroll.innerHeight($(categories));
        expect(innerHeight).toBeGreaterThan(0);
    });
    it("has an inner height smaller than the scroll height", function () {
        var innerHeight = scroll.innerHeight($(categories));
        expect(innerHeight).toBeLessThan(categories.scrollHeight);
    });

Cross-browser test refinements

Due to browser differences, the distance between elements test has fractional differences, so we’ll use toBeCloseTo set to zero decimal places, to help ensure consistency, and separate the two expect tests too.

    it("gets the distance between two neighboring elements", function () {
        var $el1 = $("p:first", $("#categories"));
        var $el2 = $el1.next("p");
        expect(scroll.outerDistanceBetween($el1, $el2)).toBeCloseTo(0, 0);
    });
    it("gets the distance between two elements separated by another element between them", function () {
        var $el1 = $("p:first", $("#categories"));
        var $el2 = $el1.next("p").next("p");
        expect(scroll.outerDistanceBetween($el1, $el2)).toBeCloseTo(-$el1.get(0).offsetHeight, 0);
    });

Testing scroll manager

With the scroll manager code, I’ve gone through the existing code looking for all the decisions that the code makes, and have put together an outline for the tests:

/*jslint browser:true */
/*global $, scrollManager, describe, beforeEach, afterEach, it, expect */
describe("Scroll manager", function () {
    "use strict";
    describe("arrows", function () {
        xit("arrows up to the previous element", function () {
        });
        xit("arrows down to the next item", function () {
        });
        xit("arrows down to the next element", function () {
        });
        xit("doesn't arrow down from the last visible element", function () {
        });
    });
    describe("home and end", function () {
        xit("goes home to the start", function () {
        });
        xit("can move home to the start without moving the focus", function () {
        });
        xit("moves to the end", function () {
        });
        xit("ignores hidden fields", function () {
        });
        xit("ignores disabled fields", function () {
        });
        xit("focuses the label on the input", function () {
        });
        xit("shouldn't focus when no element is given", function () {
        });
        xit("stops searching for scroll child at HTML element", function () {
        });
        xit("searches for scroll child", function () {
        });
    });
    describe("page up and page down", function () {
        xit("moves the page down focus instead of scrolling", function () {
        });
        xit("scrolls when page down focus is at the bottom", function () {
        });
        xit("moves the page up focus instead of scrolling", function () {
        });
        xit("scrolls when the page up focus is at the top", function () {
        });
    });
    xit("selects the clicked category item", function () {
    });
});

I’m using xit here instead of it, so that the test doesn’t run but is included as “pending”, so that it appears in the list, and I’ll change them from xit to it, when each test is ready to run.

I can use the same technique as with other testing, to add categories HTML code before each test and remove it after each test, so that we have the same consistent set of categories to test with.

    beforeEach(function () {
        categories = addCategories();
    });
    afterEach(function () {
        categories = removeCategories();
    });

Testing Move to previous

We can test if the arrow up does its thing, by checking that the moveToPrevious() function returns the previous label.

        it("arrows up to the previous element", function () {
            var items = categories.children;
            var el = items[items.length - 1].querySelector("input");
            var prevInput = items[items.length - 2].querySelector("input");
            el = scrollManager.moveToPrevious(el);
            expect(el.get(0).id).toBe(prevInput.id);
        });

I want to do this in a better way though. I want to use the keydownHandler() function to test the arrow up event, but currently it doesn’t return anything. A simple change is to have it return the result of the function that it calls.

Alternatively, I could investigate to find out which is the currently focused element, for which I can use document.activeElement

        it("arrows up to the previous element", function () {
            var items = categories.children;
            var el = items[items.length - 1].querySelector("input");
            var prevInput = items[items.length - 2].querySelector("input");
            scrollManager.moveToPrevious(el);
            expect(document.activeElement.id).toBe(prevInput.id);
        });

Now that I don’t have to use the returned element from the moveToPrevious() function, I still want to go ahead with a different improvement to the function.

Currently it is using keyCode to find out which key was pressed:

        if (evt.keyCode === 38) {
            evt.preventDefault();
            return moveToPrevious(el);
        }

The use of keyCode is deprecated, and I want to use key instead, which gives a text representation such as “ArrowUp”

    function keydownHandler(evt) {
        var el = evt.target;
        if (evt.key === "PageUp") {
            pageUpFrom(el);
            evt.preventDefault();
        }
        if (evt.key === "PageDown") {
            pageDownFrom(el);
            evt.preventDefault();
        }
        if (evt.keyCode === "End") {
            moveToEnd(el);
            evt.preventDefault();
        }
        if (evt.keyCode === "Home") {
            moveToStart(el);
            evt.preventDefault();
        }
        if (evt.key === "ArrowUp") {
            moveToPrevious(el);
            evt.preventDefault();
        }
        if (evt.keyCode === "ArrowDown") {
            moveToNext(el);
            evt.preventDefault();
        }
    }

That will still work with the existing code, and means that the tests can now be closer to how the scroll manager is actually used, by using the keydownHandler() function to control things instead.

We can now use a dummy event object, and assign to it the target element and the key when testing:

    var dummyEvt = {
        preventDefault: function () {
            return;
        }
    };
...
        it("arrows up to the previous element", function () {
            var items = categories.children;
            var currentEl = items[items.length - 1].querySelector("input");
            var expectedEl = items[items.length - 2].querySelector("input");
            var evt = Object.assign({
                target: currentEl,
                key: "ArrowUp"
            }, dummyEvt);
            scrollManager.keydownHandler(evt);
            expect(document.activeElement.id).toBe(expectedEl.id);
        });

That test is a lot clearer now. The intention of the test is now crystal clear, making it much easier to understand that we are interested in what the active element is, when the up arrow is pressed.

Testing that moving up from the first item remains on the first item

Going up from the first item should result in you remaining on that first item.

        it("doesn't arrow up from the first element", function () {
            var items = categories.children;
            var currentEl = items[0].querySelector("input");
            var expectedEl = items[0].querySelector("input");
            var evt = Object.assign({
                target: currentEl,
                key: "ArrowUp"
            }, dummyEvt);
            scrollManager.keydownHandler(evt);
            expect(document.activeElement.id).toBe(expectedEl.id);
        });

Testing Move to next

We can now easily test that arrowing down behaves appropriately too.

        it("arrows down to the next item", function () {
            var items = categories.children;
            var currentEl = items[0].querySelector("input");
            var expectedEl = items[1].querySelector("input");
            var evt = Object.assign({
                target: currentEl,
                key: "ArrowDown"
            }, dummyEvt);
            scrollManager.keydownHandler(evt);
            expect(document.activeElement.id).toBe(expectedEl.id);
        });

And that when on the last item in the list, that arrowing down doesn’t do anything strange and leaves you on the last item in the list.

        it("doesn't arrow down from the last visible element", function () {
            var items = categories.children;
            var currentEl = items[items.length - 1].querySelector("input");
            var expectedEl = items[items.length - 1].querySelector("input");
            var evt = Object.assign({
                target: currentEl,
                key: "ArrowDown"
            }, dummyEvt);
            scrollManager.keydownHandler(evt);
            expect(document.activeElement.id).toBe(expectedEl.id);
        });

That’s the scroll manager tests all in place. Next up I’ll work on Home/End, and Page Up/Down.

5 Likes

Before moving on with other tests for Home/End/PageUp/PageDown, I need to refactor the testing code.
Currently there is a lot of duplication that needlessly distracts from conveying the desired intent.

Virtually all of the scrollmanager tests will involve the list of items, so the beforeEach section is a good place to assign those each time.

    var categories;
    var items;
...
    beforeEach(function () {
        categories = addCategories();
        items = categories.children;
    });

Much of the existing test can also be moved out to a separate function. Coming up with good function names is hard. I was going to call it testKeyItem, but that doesn’t tell you enough, so I’m calling it issueItemKeyCommand() instead:

    function issueItemKeyCommand(target, key) {
        var evt = makeDummyEvt({target, key});
        scrollManager.keydownHandler(evt);
        return document.activeElement;
    }

Each test is now much simpler, and easier to digest:

        it("arrows up to the previous element", function () {
            var currentItem = items[items.length - 1].querySelector("input");
            var expectedItem = items[items.length - 2].querySelector("input");
            var item = issueItemKeyCommand(currentItem, "ArrowUp");
            expect(item.id).toBe(expectedItem.id);
        });

Testing Home and End

Because Home and End require doing two different things, one of selecting an item, and the other of scrolling so that the item can be seen, two separate tests are useful for each of these.

        it("selects the last item in the list", function () {
            var currentItem = items[0].querySelector("input");
            var expectedItem = items[items.length - 1].querySelector("input");
            var item = issueItemKeyCommand(currentItem, "End");
            expect(item.id).toBe(expectedItem.id);
        });

The best way to check that End has scrolled, is to check that scrollTop contains an appropriate value. We can start with the scroll at zero, and check to see that it has scrolled to the end by using the total height of the container, and subtracting from that the visible height of the container (called offset height). The remainder is how far it must scroll.

        it("moves to the end of the list", function () {
            var currentItem = items[0].querySelector("input");
            var scrollLength = categories.scrollHeight - categories.offsetHeight;
            categories.scrollTop = 0;
            issueItemKeyCommand(currentItem, "End");
            expect(categories.scrollTop).toBe(scrollLength);
        });

Now that we have tests that make sure End works correctly, we can go from the first item to the last item, then check if issuing the Home command brings us back to the first item.

        it("selects the first item in the list", function () {
            var firstItem = items[0].querySelector("input");
            var lastItem = issueItemKeyCommand(firstItem, "End");
            var item = issueItemKeyCommand(lastItem, "Home");
            expect(item.id).toBe(firstItem.id);
        });

Testing PageUp and PageDown

Testing PageDown is easy at first, for we just need to check that the focus moves to a different item.

        it("moves the page down focus instead of scrolling", function () {
            var firstItem = items[0].querySelector("input");
            var pageDownItem = issueItemKeyCommand(firstItem, "PageDown");
            expect(pageDownItem.id).not.toBe(firstItem.id);
        });

The second time that PageDown occurs, the focus should already be on the last visible item, so things should scroll down to the next items.

        it("scrolls when page down focus is at the bottom", function () {
            var firstItem = items[0].querySelector("input");
            var pageDownItem = issueItemKeyCommand(firstItem, "PageDown");
            issueItemKeyCommand(pageDownItem, "PageDown");
            expect(categories.scrollTop).toBeGreaterThan(0);
        });

Testing PageUp just means going to the end and seeing if we end up on a different item.
I don’t want to get too detailed with the tests. If further testing is needed, that then is when i’s a good time to get in to further detail.

        it("moves the page up focus instead of scrolling", function () {
            var firstItem = items[items.length - 1].querySelector("input");
            var endItem = issueItemKeyCommand(firstItem, "End");
            var pageUpItem = issueItemKeyCommand(endItem, "PageUp");
            expect(pageUpItem.id).not.toBe(endItem.id);
        });

I’m checking that when doing PageUp twice from the end of the list, that the item list is somewhere between the start and the end.

        it("scrolls when the page up focus is at the top", function () {
            var firstItem = items[items.length - 1].querySelector("input");
            var endItem = issueItemKeyCommand(firstItem, "End");
            var scrollLength = categories.scrollHeight - categories.offsetHeight;
            var pageUpItem = issueItemKeyCommand(endItem, "PageUp");
            issueItemKeyCommand(pageUpItem, "PageUp");
            expect(categories.scrollTop).toBeGreaterThan(0);
            expect(categories.scrollTop).toBeLessThan(scrollLength);
        });

That’s the main parts of Home/End/PageUp/PageDown all tested.

Next up is to look at what else in ScrollManager needs tests, for which code coverage will be used to help find the remaining untested sections of code.

3 Likes

Chrome has some nice code coverage abilities now. I won’t detail how they’re used, but full details are found in the Using the Chrome devtools new code coverage feature article.

Removing unused canFocus() function

The first section of code with no coverage is in the canFocus() function:

    // canFocus() source - https://stackoverflow.com/questions/18261595/how-to-check-if-a-dom-element-is-focusable/18261861
     function canFocus($el) {
        if ($el.is(":hidden") || $el.is(":disabled")) {
            return false;
        }
        ...

I couldn’t create a test to exercise that code, and on further investigation I found out why. Other code checks if the item is visible before using it, so the hidden part isn’t used, and none of the existing usage of the search results ends up in having disabled items.

None of my search result interactions result in any different values coming from the canFocus function, so I’m removing that function.

    // function canFocus($el) {
    //    ...
    // }
    ...
        // if (!canFocus($el) && $el.focusable().length > 0) {
        if ($el.focusable().length > 0) {

Unused isLabel code

This code is also not used:

    function setFocused($el) {
        ...
        if ($el.is("label") && $el.attr("for")) {
            return setFocused("#" + $el.attr("for"));
        }

because the only other code that calls setFocused, now makes sure that we go up to the scroll child element.

That means that we can completely remove that if statement, because it has no effect on anything anymore.

        // if ($el.is("label") && $el.attr("for")) {
        //     return setFocused("#" + $el.attr("for"));
        // }

More mystery code

The next code shown by the coverage tool checks if we have no elements:

        if ($el.length < 1) {
            return;
        }

and I don’t know how to normally end up with no elements. What’s missing here is an understanding of why this code is in there.
If proper procedure had been followed, we would have a test that can help us to understand the purpose of this code.

Going back to the original post, I find that it’s left over from earlier code, and isn’t needed anymore.
The scrollmanager code can only be triggered by interacting with an item in the list, which results in there always being at least one item in the list, so this check can be removed too.

    function setFocused($el) {
        ...
        // if ($el.length < 1) {
        //     return;
        // }
        ...
    }

Simplify upToScrollChild() function

We can also remove a similar check from the upToScrollChild() function.

    function upToScrollChild($el) {
        ...
        // if ($el.length === 0) {
        //     return;
        // }
        ...
    }

If any problems ever occur because these checks have been removed, we can then add a test to properly document the problem and add the code back in.

I also saw in that function that the condition for the while loop looked somewhat confusing.
Instead of changing that now though, I’m going to carry on with building the unit tests, and leave a TODO comment to remind me to come back to deal with this afterwards.

    function upToScrollChild($el) {
        // TODO: reorganise the condition
        while (
            !(hasScrollContainerAsParent($el) || $el.is("html"))
        ) {
        ...
    }

Removing noFocus() function

The noFocus part of the code isn’t used anymore either, so removing that also needs to be done.

    // function moveTo($el, noFocus) {
    function moveTo($el) {
        ...
        // if (noFocus) {
        //     return;
        // }
        ...
    }
    // function moveToStart(el, noFocus) {
    function moveToStart(el) {
        ...
        // return moveTo($newEl, noFocus);
        return moveTo($newEl);
    }

Simplifying most of moveToGivenDirection()

Next up in the moveToGivenDirection() function, we have a while condition that never gets used.

    function moveToGivenDirection($el, getElFunc) {
        ...
        if (getElFunc($newEl).length > 0) {
            $newEl = getElFunc($newEl);
            while (!$newEl.is(":visible") && getElFunc($newEl).length > 0) {
                $newEl = getElFunc($newEl);
            }
            if (!$newEl.is(":visible")) {
                $newEl = $el;
            }
        }
        ...
    }

Is the getElFunc() function ever able to give a non-visible element? The moveToPrevious() function is used as the getElFunc() function and doesn’t protect against non-visible elements, while the moveToNext() function does protect.

    function moveToPrevious(el) {
        return moveToGivenDirection($(el), function getPrev($el) {
            return $el.prev();
        });
    }
    function moveToNext(el) {
        return moveToGivenDirection($(el), function getNext($el) {
            return $el.nextAll(":visible").first();
        });
    }

The following test for arrowing up past invisible elements, results in properly exercising the while loop.

        it("skips up over hidden items", function () {
            items[1].setAttribute("style", "display: none");
            var currentItem = items[2].querySelector("input");
            var expectedItem = items[0].querySelector("input");
            var item = issueItemKeyCommand(currentItem, "ArrowUp");
            expect(item.id).toBe(expectedItem.id);
        });

A decision can now be made. Do we have both the moveToPrevious() and moveToNext() functions checking for visible elements, and double up on that with a useless while loop? That doesn’t seem to be efficient.

We could remove the visible element check from the moveToPrevious() and moveToNext() functions and keep the check in the while loop, but that results in poor efficiency in a long list of items where all but the top and bottom are invisible.

Instead, we can remove the while loop and have the moveToPrevious() and moveToNext() functions give only the next visible item.

        if (getElFunc($newEl).length > 0) {
            $newEl = getElFunc($newEl);
            // while (!$newEl.is(":visible") && getElFunc($newEl).length > 0) {
            //     $newEl = getElFunc($newEl);
            // }
            if (!$newEl.is(":visible")) {
                $newEl = $el;
            }
        }

Doing that results in the tests continuing to pass, and reveals another improvement that can be made. Here’s the updated code:

        if (getElFunc($newEl).length > 0) {
            $newEl = getElFunc($newEl);
            if (!$newEl.is(":visible")) {
                $newEl = $el;
            }
        }

We are checking if the getElFunc() function has a length greater than zero, which only occurs when there are visible elements available. Then we are checking if the element is not visible, which is completely wasted as it’s already being done by checking the length.

We can improve on this code by removing that second check:

        if (getElFunc($newEl).length > 0) {
            $newEl = getElFunc($newEl);
            // if (!$newEl.is(":visible")) {
            //     $newEl = $el;
            // }
        }

And lastly, we are checking if getElFunc() contains anything before running that function again to assign it to a variable.

We should only run that function once, and then check if it contains anything. If it doesn’t contain anything we can the move to the existing element instead:

        $newEl = getElFunc($newEl);
        if ($newEl.length > 0) {
            $el = $newEl;
        }
        return moveTo($el);

That’s a lot simplifications made to the code, but the tests that are already in place help to give us instant feedback that nothing breaks, allowing us to easily make changes to the code without fear of breaking something.

3 Likes

The Code Coverage checks reveal that there’s more cleaning up to be done to the scrollmanager code.

Cleaning up PageUp and PageDown

In the PageUp and PageDown sections, we have code that used to serve a purpose but no longer gets run.

    function getPageUpItem($el) {
        ...
        return ($filteredScrollItems.length >= 1)
            ? $filteredScrollItems.first()
            : $container.children().first();
    }

It could be that my existing tests are not complete, so an investigation to find out if it’s possible to exercise this code using tests must be done.

On investigation though, the filteredScrollItems gives multiple items from the list, and as the even is triggered from one of the items, can never be less than one item.

That ternary clause is no longer needed, and can never be triggered now, thus it must go.

    function getPageUpItem($el) {
        ...
        // return ($filteredScrollItems.length >= 1)
        //     ? $filteredScrollItems.first()
        //     : $container.children().first();
        return $filteredScrollItems.first();
    }

And the same occurs in the getPageDownItem() function too.

    function getPageDownItem($el) {
        ...
        // return ($filteredScrollItems.length >= 1)
        //     ? $filteredScrollItems.last()
        //     : $container.children().last();
        return $filteredScrollItems.last();

Cleaning up Home and End

The moveToStart() and moveToEnd() functions both use different techniques, from which useful information can be learned.

The moveToStart() function ends with:

        $newEl = $newEl.prevAll(":visible").last();
        if ($newEl.length > 0) {
            el = $newEl;
        }
        return moveTo($(el));

I don’t like how the $newEl jQuery object is being reassigned to the el variable.
I also don’t like how $(el) is needed for the moveTo function.

The updated code removes those problems by reversing the condition, so that we don’t fix the problem until the problem is there.

        $newEl = $newEl.prevAll(":visible").last();
        if ($newEl.length === 0) {
            $newEl = $(el);
        }
        return moveTo($newEl);

The moveToEnd() function ends with:

        while ($newEl.nextAll(":visible").first().length) {
            $newEl = $newEl.nextAll(":visible").first();
        }
        if (!$newEl.length) {
            $newEl = $(el);
        }
        return moveTo($newEl);

I don’t like how nextAll is called twice, and the coverage test shows that the if statement isn’t used.
Also, first get the next available element, when we should use last to get the last one instead.

        $newEl = $newEl.nextAll(":visible").last();
        if ($newEl.length === 0) {
            $newEl = $(el);
        }
        return moveTo($newEl);

The existing tests were used throughout when exploring options for this new code, which helped to give assurance that everything that we care about hasn’t broken due to these updates.

Testing the click handler

There’s only one last thing in the scrollmanager code that doesn’t yet have coverage, and that’s the clickWrapper() and clickHandler() functions. These are really easy to test. We can just check if clicking results in the focus moving from one item to another item.

    it("selects the clicked category item", function () {
        var firstItem = items[0].querySelector("input");
        var secondItem = items[1].querySelector("input");
        firstItem.click();
        expect(document.activeElement).toBe(firstItem);
        secondItem.click();
        expect(document.activeElement).toBe(secondItem);
    });

Refactoring some code

The upToScrollChild() function can do with some improvement, because the condition in the while loop is somewhat difficult to currently understand:

    function upToScrollChild($el) {
        // TODO: reorganise the condition
        while (
            !(hasScrollContainerAsParent($el) || $el.is("html"))
        ) {
            $el = $el.parent();
        }
        return $el;
    }

I want to remove the exclamation mark in front of the parenthesis, because it can be easier to understand when it’s in front of each relevant condition instead.
I also want to bring the “html” part to the front of the condition, for it’s not allowed to continue until that condition has been met.

None of this changes the overall behaviour of the code. Instead, because we are refactoring the code we can just check that all of the tests still continue to pass throughout the refactoring process.

It helps here to know that !(a || b) is the same as (!a && !b)

    function upToScrollChild($el) {
        while (!$el.is("html") && !hasScrollContainerAsParent($el)) {
            $el = $el.parent();
        }
        return $el;
    }

That makes the condition a lot easier to understand now.

Scroll manager testing complete

With these changes, the scrollmanager code is now 100% fully covered by tests. We can now ensure that any changes that we want to do to the code are documented first with a test, followed by writing the code that makes the test pass.

3 Likes

What are the rest of the scripting files that need tests?

The scripting files loaded by the index page are:

<script src="https://code.jquery.com/jquery-3.1.1.js"></script>
<script src="lib/escaperegexp.js"></script>
<script src="lib/jquery.focusable.js"></script>
<script src="src/queue.js"></script>
<script src="src/scroll.js"></script>
<script src="src/scrollmanager.js"></script>
<script src="src/filtercheckboxesbytext.js"></script>
<script src="src/checkboxlist.js"></script>
<script src="src/resultscontroller.js"></script>
<script src="src/searchresults.js"></script>

jQuery and the lib scripts don’t need tests from us, as those libraries should already have their own tests.
It’s best to write tests only for the code that you are changing.

We have created tests for queue, scroll, and scrollmanager, but none exist for the others yet.
It would have been much easier to have created them as we wrote the code, and it would be a poor idea to write more code without tests to ensure that things remain working, so creating the missing tests is the best idea before making further advancements.

Queue test refinement

On looking back at the queue code I see that the getAll() function, even though it has full coverage, isn’t fully tested.

The code can be simplified to the following code, and it still fully passes all of the tests:

        function getAll() {
            // return list.slice(0);
            return list;
        }

Why was the slice there? It’s there to protect the array from external change, so we need a test to document this, and supply good reason for the slice to be there.

The following test checks if the array from getAll() can affect the queue:

    it("Prevents the array from being changed", function () {
        var result;
        queue.add("item");
        result = queue.getAll();
        result.length = 0;
        expect(queue.getAll()).toEqual(["item 1"]);
    });

To fix that test we just need to return a new array, which is achieved using .slice(0)

        function getAll() {
            return list.slice(0);
        }

A more modern way of doing this uses the ES6 spread syntax by returning […list] instead.

Preparing to testing filterCheckboxesByText()

It’s been difficult to motivate myself to do more testing. A lot of testing all at the same time is draining, which gives good motivation to work it in to the development process as a part of writing code instead.

The filterCheckboxesByText() function is used as follows:

    $(categories).filterCheckboxesByText($(searchField), {
        afterSearch: function doAfterSearch() {
            // $(categories).scrollTop(0);
        }
    });

The afterSearch part isn’t used by us, but will also be included in testing to ensure that the existing code really does what it’s expected to do, so that we’ll know that it works properly when we do come to use it.

The first test is for basic functionality. With a text field and some categories, adding a character to the text field should result in the categories list being filtered.

We’re going to need an input area to go with the categories that we already have.

<p><input type="text" id="mytextbox"></p>

I could add that to the addcategories.js file, but that’s then expanding beyond the interest of categories. Instead, it needs to be called something more generic like create-sandbox.js, with the functions inside being renamed to createSandbox(), addSandbox(), and removeSandbox()

// function createCategories() {
function createSandbox() {
    document.body.innerHTML += `<p><input type="text" id="mytextbox"></p>
    <div id="categories">
    ...
    </div>`;
}
// function addCategories() {
function addSandbox() {
    // createCategories();
    createSandbox();
    return document.querySelector("#categories");
}
// function removeCategories() {
function removeSandbox() {
    var categories = document.querySelector("#categories");
    categories.parentNode.removeChild(categories);
    return undefined;
}

Which means that the tests need to be updated:

    beforeEach(function () {
        // categories = addCategories();
        categories = addSandbox();
    });
    afterEach(function () {
        // categories = removeCategories();
        categories = removeSandbox();
    });

While doing that though I realised that the functions can no longer only return the categories, but should also return other relevant things like the text box, so returning an object of items is required instead.

    var categories;
    beforeEach(function () {
        var sandbox = addSandbox();
        // categories = addCategories();
        categories = sandbox.categories;
    });
    afterEach(function () {
        var sandbox = removeSandbox();
        // categories = removeCategories();
        categories = sandbox.categories;
    });

So the create-test-html code needs instead to be:

function addSandbox() {
    createSandbox();
    // return document.querySelector("#categories");
    return {
      textfield: document.querySelector("#mytextbox"),
      categories: document.querySelector("#categories")
    };
}
function removeSandbox() {
    var textfield = document.querySelector("#mytextbox");
    var categories = document.querySelector("#categories");
    textfield.parentNode.removeChild(textfield);
    categories.parentNode.removeChild(categories);
    // return undefined;
    return {
      textfield: undefined,
      categories: undefined
    };
}

Improving filenames

While getting these changes done, it’s also a good time to use a more standard naming scheme for the spec files, by renaming them to end with .spec.js instead.

      <!-- include spec files here... -->
    <script src="spec/create-sandbox.js"></script>
    <script src="spec/queue.spec.js"></script>
    <script src="spec/scroll.spec.js"></script>
    <script src="spec/scrollmanager.spec.js"></script>

We’re now in a good place to add some tests for the filterCheckboxesByText() function, which will let us move on to testing checkboxlist, resultscontroller, and finally searchresults.

3 Likes

Refining the sandbox

Seeing the use of addSandbox() and removeSandbox(), it would be nice if the sandbox was an object that had add and remove methods on it.
Creating a sandbox.js and moving most of the code from create-sandbox.js to just sandbox.js makes good sense too.

    <script src="spec/sandbox.js"></script>
    <script src="spec/create-sandbox.js"></script>

We can now rework sandbox.js to be an object with methods instead.

Test before writing code

But - before writing code we must write a test instead. This is a good opportunity to use tests to help define how we want to use the sandbox.
This process of thinking about the tests for the code is quite beneficial too. I was wanting the main object to be called sandbox, and yet when we initialize things we want to assign it to a variable called sandbox too.

Fortunately, thinking about how to test this has helped me to realise that inside the sandbox there is the sandpit, which we can use instead.

Init test and code

There are two main things that the sandbox needs to do. We need to init the sandbox, and we need to reset the sandbox back to its initial state.

it("Accepts an initial condition for the sandbox", function () {
    var html = `<div id="anElement"></div>`;
    var sandpit = sandbox.init(html);
    expect(sandpit.innerHTML).toBe(html);
});

The following code is enough to make that test pass:

var sandbox = (function iife() {
    var sandpit = document.createDocumentFragment();
    function initSandpit(html) {
        sandpit.innerHTML = html;
        return sandpit;
    }
    return {
        init: initSandpit
    };
});

Reset test and code

And resetting the sandbox just means changing (or emptying) the sandpit, and seeing if resetting returns it back to how it started.

    it("Resets the sandbox", function () {
        var html = `<div id="anElement"></div>`;
        var sandpit = sandbox.init(html);
        sandpit.innerHTML = "";
        sandpit = sandbox.reset();
        expect(sandpit.innerHTML).toBe(html);
    });

Because the reset needs to know the initial html, so we need to save that initial html in the init function:

    var initHtml = "";
    var sandpit = document.createDocumentFragment();
    function initSandpit(html) {
        initHtml = html;
        sandpit.innerHTML = html;
        return sandpit;
    }

which lets us use that initHtml from the reset function:

    function resetSandpit() {
        sandpit.innerHTML = initHtml;
        return sandpit;
    }
    ...
    return {
        reset: resetSandpit,
        ...
    };

Refactor code

And now that the code is passing those tests, we can refactor to improve the code, by having the init function call the reset one instead.

    function initSandpit(html) {
        initHtml = html;
        return resetSandpit();
    }

And that’s all there seems to be for the sandbox.

var sandbox = (function iife() {
    var initHtml = "";
    var sandpit = document.createDocumentFragment();
    function resetSandpit() {
        sandpit.innerHTML = initHtml;
        return sandpit;
    }
    function initSandpit(html) {
        initHtml = html;
        return resetSandpit();
    }
    return {
        reset: resetSandpit,
        init: initSandpit
    };
}());

Adding sandbox to the document

But, when using the sandbox for the tests, some of the tests break because it seems that querySelector isn’t able to query a document fragment.
We need to out the sandbox in the document, so that querySelector can access the elements that we need.

var sandbox = (function iife() {
    var initHtml = "";
    // var sandpit = document.createDocumentFragment();
    var sandpit;
    ...
    function addSandbox() {
      var div = document.createElement("div");
      div.id = "sandbox";
      document.body.appendChild(div);
    }
    function initSandpit(html) {
        initHtml = html;
        sandpit = document.body.querySelector("#sandbox");
        if (!sandpit) {
          addSandbox();
          sandpit = document.body.querySelector("#sandbox");
        }
        return resetSandpit();
    }
    ...
}());

Create sandbox

Using sandbox.init() when testing the sandbox interferes with the other existing create-sandbox.js code, so putting that other code in a separate function so that we can call it when we need it, results in everything working well once again.

function createSandbox() {
  return sandbox.init(`<p><input type="text" id="mytextbox"></p>
    <div id="categories">
    ...
    </div>`);
}

Using the sandbox

We can now use the sandbox on the other tests. Because we are using reset, we don’t need the afterEach section anymore either.
Here’s how it’s used on the scroll test:

    beforeEach(function () {
        sandpit = createSandbox();
        categories = sandpit.querySelector("#categories");
    });
    // afterEach(function () {
    //     sandpit = sandbox.remove();
    // });

Similar occurs with the scrollmanager tests too:

    beforeEach(function () {
        sandpit = sandbox.reset();
        categories = sandpit.querySelector("#categories");
        items = categories.children;
    });
    // afterEach(function () {
    //     sandpit = sandbox.remove();
    // });

All in all, the full code for the sandbox is:

var sandbox = (function iife() {
    var initHtml = "";
    var sandpit;
    function resetSandpit() {
        sandpit.innerHTML = initHtml;
        return sandpit;
    }
    function addSandbox() {
      var div = document.createElement("div");
      div.id = "sandbox";
      document.body.appendChild(div);
    }
    function initSandpit(html) {
        initHtml = html;
        sandpit = document.body.querySelector("#sandbox");
        if (!sandpit) {
          addSandbox();
          sandpit = document.body.querySelector("#sandbox");
        }
        return resetSandpit();
    }
    return {
        reset: resetSandpit,
        init: initSandpit
    };
}());

We’re now ready to carry on testing, and have this handy sandbox to replace the create-categories code from earlier.
The tests made it really easy to create the code for a sandbox that does exactly what we wanted to do.

3 Likes

Thanks @TechoKid for getting in touch about how you appreciate this. That helps a lot.

The filterCheckboxesByText() code is the next one that needs test code.

Testing filterCheckboxesByText()

When testing the filterCheckboxesByText() function, it helps to have something simple in the sandbox for testing, so just using One, Two, Three, will be enough to help exercise the code.

describe("Filter checkboxes by text", function () {
    var sandpit;
    var input;
    var categories;
    beforeEach(function () {
        sandpit = sandbox.init(`<p><input type="text"></p>
    <div id="categories">
      <p>
        <input id="One" type="checkbox" value="One">
        <label for="One">One</label>
      </p>
      <p>
        <input id="Two" type="checkbox" value="Two">
        <label for="Two">Two</label>
      </p>
      <p>
        <input id="Three" type="checkbox" value="Three">
        <label for="Three">Three</label>
      </p>
    </div>`);
        input = sandpit.querySelector("input");
        categories = sandpit.querySelector("#categories");
    });
    ...
});

First things first

The first thing to check is that the filterCheckboxesByText() function actually exists:

    it("Has filterCheckboxesByText() on the jQuery .fn object", function () {
        expect(typeof $.fn.filterCheckboxesByText).toBe("function");
    });

Hiding elements

The main expected behaviour is that when the input field is updated, that the non-matching fields get hidden.

We’ll want to use a separate function to trigger the keyup event.

    function keyUp(el, key) {
        var event = document.createEvent('Event');
        event.keyCode = key;
        event.initEvent('keyup');
        el.dispatchEvent(event);
    }

and when that event occurs, there should be some items hidden.
In this case, both the uppercase O of One and the lowercase o of Two should not be hidden.

    it("Hides some matching elements when triggered by a keyup event", function () {
        $(categories).filterCheckboxesByText($(input));
        input.value = "o";
        keyUp(input);
        expect($("p:not(:hidden)", categories).length).toBe(2);
    });

Switching shown items

It should also reveal previously hidden ones and hide others, when the input changes.

    it("Hides other elements when showing different ones", function () {
        $(categories).filterCheckboxesByText($(input));
        input.value = "w";
        keyUp(input);
        expect($("p:not(:hidden) label", categories).text()).toBe("Two");
        input.value = "r";
        keyUp(input);
        expect($("p:not(:hidden) label", categories).text()).toBe("Three");
    });

Make matching characters bold

The other thing that the filterCheckboxesByText() function does is to mark in bold the matching characters.

    it("Changes the label to bold matching chars", function () {
        $(categories).filterCheckboxesByText($(input));
        input.value = "tw";
        keyUp(input);
        var label = $("p:not(:hidden) label", categories);
        expect($("p:not(:hidden) label", categories).html()).toBe("<b>Tw</b>o");
    });

Removing unwanted code

The rest of the function that deals with showMatch and afterSearch aren’t used and can be removed, and the tests helps to ensure that the changes don’t change anything that we care about with the rest of the code.

        // var showMatch = (opts && opts.showMatch) || true;
        ...
            // if (showMatch === true) {
            highlightMatches(search, container);
            // }
            // if (typeof opts.afterSearch === "function") {
            //     opts.afterSearch();
            // }

The resulting code

The tests after all of this is:

describe("Filter checkboxes by text", function () {
    var sandpit;
    var categories;
    var input;

    beforeEach(function () {
        sandpit = sandbox.init(`<p><input type="text"></p>
    <div id="categories">
      <p>
        <input id="One" type="checkbox" value="One">
        <label for="One">One</label>
      </p>
      <p>
        <input id="Two" type="checkbox" value="Two">
        <label for="Two">Two</label>
      </p>
      <p>
        <input id="Three" type="checkbox" value="Three">
        <label for="Three">Three</label>
      </p>
    </div>`);
        input = sandpit.querySelector("input");
        categories = sandpit.querySelector("#categories");
    });
    function keyUp(el, key) {
        var event = document.createEvent('Event');
        event.keyCode = key;
        event.initEvent('keyup');
        el.dispatchEvent(event);
    }

    it("Has filterCheckboxesByText on the jQuery .fn object", function () {
        expect(typeof $.fn.filterCheckboxesByText).toBe("function");
    });
    it("Hides some matching elements when triggered by a keyup event", function () {
        $(categories).filterCheckboxesByText($(input));
        input.value = "o";
        keyUp(input);
        expect($("p:not(:hidden)", categories).length).toBe(2);
    });
    it("Hides other elements when showing different ones", function () {
        $(categories).filterCheckboxesByText($(input));
        input.value = "w";
        keyUp(input);
        expect($("p:not(:hidden)", categories).length).toBe(1);
        expect($("p:not(:hidden) label", categories).text()).toBe("Two");
        input.value = "r";
        keyUp(input);
        expect($("p:not(:hidden)", categories).length).toBe(1);
        expect($("p:not(:hidden) label", categories).text()).toBe("Three");
    });
    it("Changes the label to bold matching chars", function () {
        $(categories).filterCheckboxesByText($(input));
        input.value = "tw";
        keyUp(input);
        var label = $("p:not(:hidden) label", categories);
        expect($("p:not(:hidden) label", categories).html()).toBe("<b>Tw</b>o");
    });
});

And the resulting code that passes those tests is:

jQuery(function ($) {
    $.fn.filterCheckboxesByText = function filterCheckboxesByText(searchField, opts) {
        var container = this;
        var search;

        function containsText(haystack, needle) {
            return needle.toLowerCase().indexOf(haystack.toLowerCase()) > -1;
        }
        function compareWithSearch(ignore, checkbox) {
            var label = $(checkbox).next("label");
            return containsText(search, label.text());
        }
        function caseInsensitiveBold(text, search) {
            var searchRx = new RegExp("(" + escapeRegExp(search) + ")", "gi");
            return text.replace(searchRx, "<b>$1</b>");
        }
        function highlightMatches(search, container) {
            $("label", container).each(function highlightMatchOnLabel(ignore, label) {
                $(label).html(caseInsensitiveBold($(label).text(), search));
            });
        }
        function showItem(ignore, checkbox) {
            $(checkbox).parent("p").show();
        }

        $(searchField).bind("change keyup", function searchChangeHandler(evt) {
            search = $(evt.target).val();

            $("p", container).hide();

            $(":checkbox", container)
                .filter(compareWithSearch)
                .each(showItem);
            highlightMatches(search, container);
        });
    };
});

The remaining code that needs to be tested is for:

  • checkboxlist.js
  • resultscontroller.js
  • searchresults.js

and checkboxlist.js is what we’ll move on to testing next time.

3 Likes

Testing checkboxlist.js

This code gets more difficult to test than the others because there’s less blackbox stuff to test with a function accepting arguments and returning a result, and is instead more focused on setting and dealing with events instead.

The init() method sets up two different events
The checkFirst() method potentially toggles an item, and sets the focus to that item
And, uncheckByValue() uses the value to uncheck an item.

Test the init method

We can check if the init function results in the doAfterClick function being assigned to the checkbox click event.

To check that, we can create an opts object and spy on it, checking if the doAfterClick function has been called.

    beforeEach(function () {
        ...
        opts = {
            container: categories,
            doAfterClick: function () {
                return;
            }
        };
    });
    it("executes doAfterClick when an input is clicked", function () {
        spyOn(opts, "doAfterClick");
        checkboxList.init(opts);
        expect(opts.doAfterClick).not.toHaveBeenCalled();
        categories.querySelector("input").click();
        expect(opts.doAfterClick).toHaveBeenCalled();
    });

Test keydown

The keydown event should end up calling the keydownHandler.

    function keyDown(el, key) {
        var event = document.createEvent('Event');
        event.keyCode = key;
        event.initEvent('keydown');
        el.dispatchEvent(event);
    }
    it("calls the keydownHandler on a key click", function () {
        spyOn(scrollManager, "keydownHandler");
        checkboxList.init(opts);
        expect(scrollManager.keydownHandler).not.toHaveBeenCalled();
        keyDown(categories, " ");
        expect(scrollManager.keydownHandler).toHaveBeenCalled();
    });

Testing checkbox visibility

I’ve had trouble making sense of this test which means that I’m trying to test too much. As a result I’ve been putting off trying to test it again, which is a bad sign.

How to deal with that is to recognise that it’s difficult, and to break it down into several different tests instead, which I’ll come to next.

3 Likes

The checkbox visibility was difficult to test. Normally we’re supposed to write the test first so that we can then write code that makes the test pass. In this case, I don’t have enough knowledge about what’s supposed to happen, so exploratory actions will need to occur instead.

Exploring the problem

One of the problems is that when testing on Firefox and Chrome, a test that works on one doesn’t work on the other,
Another problem is that when one test works, adding more tests results in the first one failing.

Understanding that there was cross-contamination across tests helped me to realize that there were some things that I wasn’t accounting for.

After logging some values, I found that the checkFirstCheckbox() function sometimes had an empty container variable.

    function checkFirstCheckbox() {
        console.log("checkFirst container", container);
        var $firstItem = getFirstVisible(container);
        ...
    }

That container variable gets set in the init function:

    function init(opts) {
        container = opts.container;
        var clickHandler = scrollManager.clickWrapper(opts.doAfterClick);
        ...
    }

Use the init function

’
I need to make sure that I call the init function first before running the checkFirstCheckbox() function. Because it’s going to be called from a couple of different tests, I’ll put it into a separate function called initCheckboxList() so that each test doesn’t become filled with meaningless details.

    function initCheckboxList() {
        checkboxList.init({
            container: categories,
            doAfterClick: function () {
                return;
            }
        });
    }

Test the checkFirst() method

I can now init checkboxList, and ensure that everything starts off unchecked, and that the checkFirst() function causes one of them to be checked.

    it("checks the first checkbox", function () {
        initCheckboxList();
        expect($("input:eq(0)", categories).prop("checked")).toBe(false);
        expect($("input:eq(1)", categories).prop("checked")).toBe(false);
        expect($("input:eq(2)", categories).prop("checked")).toBe(false);
        checkboxList.checkFirst();
        expect($("input:eq(0)", categories).prop("checked")).toBe(true);
        expect($("input:eq(1)", categories).prop("checked")).toBe(false);
        expect($("input:eq(2)", categories).prop("checked")).toBe(false);
    });

The other checkFirst situation to test is when some of the first inputs are already hidden. That should then result in one of the latter checkboxes being checked instead.

    it("checks the first available checkbox", function () {
        initCheckboxList();
        $("p:eq(0)", categories).hide();
        $("p:eq(1)", categories).hide();
        expect($("input:eq(0)", categories).prop("checked")).toBe(false);
        expect($("input:eq(1)", categories).prop("checked")).toBe(false);
        expect($("input:eq(2)", categories).prop("checked")).toBe(false);
        checkboxList.checkFirst();
        expect($("input:eq(0)", categories).prop("checked")).toBe(false);
        expect($("input:eq(1)", categories).prop("checked")).toBe(false);
        expect($("input:eq(2)", categories).prop("checked")).toBe(true);
    });

Test the init() method

That works, but we really should test the init() function too. For that, we can create a few handler functions that we’ll use, and can request them in the initCheckboxList function too.

    var handlers;
    beforeEach(function () {
        ...
        handlers = {
            doAfterClick: function () {
                return;
            },
            keyDownHandler: function () {
                return;
            }
        };
        ...
    });

The doAfterClick() function can now be easily spread around to all of the places that currently use it.

    beforeEach(function () {
        ...
        opts = {
            container: categories,
            doAfterClick: handlers.doAfterClick
        };
    });
...
    function initCheckboxList() {
        checkboxList.init({
            container: categories,
            doAfterClick: handlers.doAfterClick
        });
    }

And we can now spy on the handler functions to ensure that they get called when we trigger the event that result in their events being fired.

    it("calls the doAfterClick handler when a checkbox is clicked", function () {
        spyOn(handlers, "doAfterClick");
        initCheckboxList();
        expect(handlers.doAfterClick).not.toHaveBeenCalled();
        $(":checkbox:first", categories).click();
        expect(handlers.doAfterClick).toHaveBeenCalled();
    });

Refactor other tests

We can also improve an existing test for the keydownHandler, by using the initCheckboxList() function that we now have.

    it("calls the keydownHandler on a key click", function () {
        spyOn(scrollManager, "keydownHandler");
        // checkboxList.init(opts);
        expect(scrollManager.keydownHandler).not.toHaveBeenCalled();
        keyDown(categories, " ");
        expect(scrollManager.keydownHandler).toHaveBeenCalled();
    });

We only have easy stuff remaining to test now.

Checkbox remains checked

The checkFirstCheckbox() method should do nothing if the first visible checkbox is already checked.

    it("leaves the checkbox checked if it's already checked", function () {
        initCheckboxList();
        $("input:eq(0)", categories).click();
        expect($("input:eq(0)", categories).prop("checked")).toBe(true);
        checkboxList.checkFirst();
        expect($("input:eq(0)", categories).prop("checked")).toBe(true);
    });

The checkCheckbox() function still shows though that part of the function is not covered by tests, which I think is the return statement.

    function checkCheckbox($checkbox) {
        if ($checkbox.is(":checked")) {
            return;
        }
        $checkbox.click();
    }

I can easily deal with that by inverting the if statement, and having no return.

    function checkCheckbox($checkbox) {
        // if ($checkbox.is(":checked")) {
        if ($checkbox.is(":not(:checked)")) {
            return;
            $checkbox.click();
        }
        // $checkbox.click();
    }

And that part of the code is now fully covered by tests.

There are only two functions left to go now, which are both dealt with by the uncheckByValue function.

Testing uncheckByValue()

    it("unchecks a checkbox by finding its value property", function () {
        var value = $("input:eq(0)", categories).val();
        initCheckboxList();
        $("input:eq(0)", categories).click();
        expect($("input:eq(0)", categories).prop("checked")).toBe(true);
        checkboxList.uncheckByValue(value);
        expect($("input:eq(0)", categories).prop("checked")).toBe(false);
    });

A similar modification is needed to the uncheckByValue() function to remove the return statement, so that full testing coverage can be achieved,

    function uncheckByValue(value) {
        var $checkbox = getCheckboxByValue(value);
        // if ($checkbox.is(":not(:checked)")) {
        if ($checkbox.is(":checked")) {
            // return;
            $checkbox.click();
        }
        // $checkbox.click();
    }

And all of the checkboxList code is now being fully tested.

There is now only the searchResults and resultsController left to go now.

3 Likes

As searchResults uses resultsController, testing resultsController is the next thing to test.

      <!-- include spec files here... -->
    ...
    <script src="spec/resultscontroller.spec.js"></script>
/*jslint browser */
/*global resultsController, describe, it, spyOn, expect */
describe("Results controller", function () {
    "use strict";
    it("...", function () {
        ...
    });
});

Test the init() function

The resultsController code has an init function, that assigns several variables and sets up a click event.

    function init(opts) {
        resultsSelector = opts.resultsSelector;
        closeButtonSelector = opts.closeButtonSelector;
        queue = opts.queue;
        $(closeButtonSelector).on("click", closeClickWrapper(opts.doAfterClose));
    }

We don’t have to worry about testing resultsSelector, or the queue, because that will occur on other tests. The closeButtonSelector and the click event though do need to be tested right now.

We’ll need to add a button to the sandbox, so that we can use that button for testing.

    var sandpit;
    beforeEach(function () {
        sandpit = sandbox.init(`<div id="results">
  <div class="mydivs1" id="Fruits">
    <div class="mydivs2">
      <button class="close">X</button>
    </div>
    <div class="mydivs3">
      <span class="myspan1">Fruits</span>
    </div>
  </div>`);
    });

We can now test that the init function is doing its job, by spying on the doAfterClick function:

    it("initializes a doAfterClose function", function () {
        var opts = {
            resultsSelector: "#results",
            closeButtonSelector: ".close",
            doAfterClose: function () {
                return;
            }
        };
        spyOn(opts, "doAfterClose");
        resultsController.init(opts);
        $(".close:first").click();
        expect(opts.doAfterClose).toHaveBeenCalled();
    });

That opts object will no doubt be refactored into a beforeEach() function, but we’ll get to that when other tests demonstrate the need for that to occur.

The resultsSelector variable that’s set in the init() function is only used by one other function that we haven’t yet tested, and that’s clearResults() function. But we can’t test that until we have some results to show, for the ones in the sandbox are automatically hidden by the existing stylesheet.

Testing showResults()

The showResults() function is fairly easy to test. We shouldn’t have any visible before we show the results, and we should have some visible after showing the results.

    it("shows results based on matching the identifier", function () {
        var results = $(opts.resultsSelector, sandpit);
        var visibleElements = $("*:visible", results);
        expect(visibleElements.length).toBe(0);
        resultsController.showResults(["Fruits"], results);
        visibleElements = $("*:visible", results);
        expect(visibleElements.length).toBeGreaterThan(0);
    });

Testing clearResults()

We can now test that clearResults() works. We know how to show results now, and that it works properly, so we can show something, and check that there is nothing there after clearing the results.

We’ll want to use that opts object again, so now is a good time to move it into the beforeEach() function.

    var opts;
    beforeEach(function () {
        opts = {
            resultsSelector: "#results",
            closeButtonSelector: ".close",
            doAfterClose: function () {
                return;
            }
        };
        ...
    });
    it("initializes a doAfterClose function", function () {
        spyOn(opts, "doAfterClose");
        ...
    });

It’ll do no harm to move the init() part into the beforeEach() function too. No, hang on, it does do harm. We cannot spy on the opts function if it’s initialized before we spy on it, so ignore that idea.

We need to use that opts object in the clearResults() test, as the function uses resultsSelector that’s assigned from the init function.

    it("hides the items in the results section", function () {
        var results = $(opts.resultsSelector, sandpit);
        var visibleElements;
        resultsController.init(opts);
        resultsController.showResults(["Fruits"], results);
        resultsController.clearResults();
        visibleElements = $("*:visible", results);
        expect(visibleElements.length).toBe(0);
    });

We know that after showResults() there are some results showing, so clearResults() we can expect a different amount, that being zero showing.

Test updateResults()

We need to test what happens when results are added for the update, when they are removed, and when things remain the same.
Because there are several related tests here, it’s a good time to create a nested describe block.

    describe("updates the results", function () {
        xit("shows an item when there was none before", function () {

        });
        xit("removes an item when it was there before", function () {

        });
        xit("shows nothing when there's nothing to show", function () {

        });
        xit("shows all items when the update changes nothing", function () {

        });
    }

We can add tests to this structure, and change xit to it when each one is ready for testing.

Testing that updateResults() shows an item

The updateResults() function accepts a queue and uses getAll() to get all of the items. Instead of creating a queue, we can simplify this by mocking up a replacement for the queue instead.

        var queueItems;
        var queue;
        beforeEach(function () {
            queueItems = ["Fruits"];
            queue = {
                getAll: function () {
                    return queueItems;
                }
            };
        });

We can then check if updateResults() works as expected:

        var visibleElements;
        it("shows an item when there was none before", function () {
            resultsController.init(opts);
            visibleElements = $("*:visible", results);
            expect(visibleElements.length).toBe(0);
            resultsController.updateResults(queue);
            visibleElements = $("*:visible", results);
            expect(visibleElements.length).toBeGreaterThan(0);
        });

This fails on the last expectation and isn’t working as expected. Investigating the results area, I see that the whole results section is hidden.

We can easily deal with that by having the showResults() function actually show the results section when any results are shown.

    function showResults(results, targetSelector) {
        results.forEach(function showResult(item) {
            ...
        });
        if (results.length > 0) { // the newly added code
            $(resultsSelector).show();
        }
    }

That might have also fixed a problem that was occurring in the original code. I’ll go back to previous messages after this post and confirm that.

Now that the test is working, we need to refactor some of the code. The visibleElements line is being duplicated all over the place, so a separate function can be used for that.

    function getVisibleElements() {
        var results = $(opts.resultsSelector, sandpit);
        return $("*:visible", results);
    }
...
        // var visibleElements = $("*:visible", results);
        // expect(visibleElements.length).toBe(0);
        expect(getVisibleElements().length).toBe(0);

The code is much cleaner after that, for example:

        it("shows an item when there was none before", function () {
            resultsController.init(opts);
            expect(visibleElements().length).toBe(0);
            resultsController.updateResults(queue);
            expect(visibleElements().length).toBeGreaterThan(0);
        });

The other updateResults() functions should just fall into place easily now. Here are the other updateResults() tests:

        it("removes an item when it was there before", function () {
            resultsController.init(opts);
            resultsController.updateResults(queue);
            expect(visibleElements().length).toBeGreaterThan(0);
            queueItems = [];
            resultsController.updateResults(queue);
            expect(visibleElements().length).toBe(0);
        });
        it("shows nothing when there's nothing to show", function () {
            resultsController.init(opts);
            expect(visibleElements().length).toBe(0);
            queueItems = [];
            resultsController.updateResults(queue);
            expect(visibleElements().length).toBe(0);
        });
        it("shows all items when the update changes nothing", function () {
            resultsController.init(opts);
            resultsController.updateResults(queue);
            expect(visibleElements().length).toBeGreaterThan(0);
            resultsController.updateResults(queue);
            expect(visibleElements().length).toBeGreaterThan(0);
        });

Removing some tests

Based on the use of updateResults(), I don’t think that showResults() or clearResults() should be public methods.
A search through the rest of the code confirms that they aren’t accessed from anywhere except by the updateResults function, so I can remove those functions from being returned out of the resultscontroller code:

    return {
        removeItem: removeItemFromQueue,
        // showResults: showResults,
        // clearResults: clearResults,
        updateResults: updateResults,
        init: init
    };

The tests associated with those functions can be removed too:

    // it("shows results based on matching the identifier", function () {
    //     var results = $(opts.resultsSelector, sandpit);
    //     expect(visibleElements().length).toBe(0);
    //     resultsController.showResults(["Fruits"], results);
    //     expect(visibleElements().length).toBeGreaterThan(0);
    // });
    // it("hides the items in the results section", function () {
    //     var results = $(opts.resultsSelector, sandpit);
    //     resultsController.init(opts);
    //     resultsController.showResults(["Fruits"], results);
    //     resultsController.clearResults();
    //     expect(visibleElements().length).toBe(0);
    // });

Cleaning up the test page

Because the results section remains displayed on the screen, I’ll empty the sandbox after all the tests have been run.

    afterAll(function () {
        sandbox.init("");
    });

Testing removeItem()

We only have one more thing to test, and that’s removing an item from the queue. This one uses the remove method, which we can mock up too.

        queueItems = ["Fruits"];
        queue = {
            getAll: function () {
                return queueItems;
            },
            remove: function (item) {
                queueItems = queueItems.filter(function (queueItem) {
                    return queueItem !== item;
                });
            }
        };
        opts = {
            ...,
            queue: queue,
            ...
        };
...
    it("can remove an item from the queue", function () {
        var itemToRemove = "Item to remove";
        queueItems = ["Fruits", itemToRemove];
        resultsController.init(opts);
        resultsController.removeItem(itemToRemove);
        expect(queueItems).toEqual(["Fruits"]);
    });

And that’s the resultsController() code fully tested. We only have the searchResults() code left to go now.

3 Likes

The last set of code to test is the searchResults code, which gets trickier. Most of the code is already executed before the test gets to it, so we need to ensure that the executed code gets tested. This includes showClearAllButton(), doAfterClick(), doAfterClose(), and the submit/click events.

To start with we’ll add a testing spec file to the SpecRunner.html file. I’ve also moved the file so that it’s inside of the spec folder, to help keep the top-level folder nice and clear.

      <!-- include spec files here... -->
    ...
    <script src="searchresults.spec.js"></script>

Based on the previous test here’s a preliminary structure for the resultsController tests.

/*jslint browser */
/*global $, checkboxList, resultsController, sandbox, describe, beforeEach, it, expect */
describe("Search results", function () {
    "use strict";
    var sandpit;
    beforeEach(function () {
        sandpit = sandbox.init(`<p><input type="text"></p>
            <div id="categories">
              <p>
                <input id="chkFruits" type="checkbox" value="Fruits">
                <label for="chkFruits">Fruits</label>
              </p>
              <p>
                <input id="chkVegetables" type="checkbox" value="Vegetables">
                <label for="chkVegetables">Vegetables</label>
              </p>
            </div>
            <p>Results: <button id="clearAll" style="display: none">Clear all results</button></p>
            <div id="results">
              <div class="mydivs1" id="Fruits">
                <div class="mydivs2">
                  <button class="close">X</button>
                </div>
                <div class="mydivs3">
                  <span class="myspan1">Fruits</span>
                </div>
              </div>
              <div class="mydivs1" id="Vegetables">
                <div class="mydivs2">
                  <button class="close">X</button>
                </div>
                <div class="mydivs3">
                  <span class="myspan1">Vegetables</span>
                </div>
              </div>
            </div>`);
    });
    it("", function () {

    });
});

Testing showClearAllButton()

The function name is somewhat misleading, because it also hides the clearAll button when there are no results. A better name that I’ve renamed it to is toggleClearAllBasedOnResults()

There are a couple of different situations in which we want to know that it works:

    it("shows the clearAll button when a result is shown", function () {

    });
    it("hide the clearAll button when no more results are shown", function () {
    });

We’ll want easy access to some common elements in the sandpit, and a function to easily check if an element is visible will be useful too:

    var categories;
    var clearAllButton;
    var results;
    beforeEach(function () {
        ...
        var categories = sandpit.querySelector("#categories");
        var clearAllButton = sandpit.querySelector("clearAll");
        var results = sandpit.querySelector("results");
    });
    function isVisible(obj) {
        return obj.offsetWidth > 0 && obj.offsetHeight > 0;
    }

We can now easily check that the clearAll button isn’t shown, and is shown when something is clicked.

    it("shows the clearAll button when a result is shown", function () {
        var item = categories.querySelector("label");
        expect(isVisible(clearAllButton)).toBe(false);
        item.click();
        expect(isVisible(clearAllButton)).toBe(true);
    });

We can also test that the clearAll button hides itself when no items are selected, by starting with a selected item and then clicking on it again to remove it.

    it("hide the clearAll button when no more results are shown", function () {
        var item = categories.querySelector("label");
        item.click();
        expect(isVisible(clearAllButton)).toBe(true);
        item.click();
        expect(isVisible(clearAllButton)).toBe(false);
    });

The rest of the tests for the searchResults code will follow in my next post.

3 Likes

While the search results worked when run with the other tests, they occasionally failed to work and when running them separately. The clearAll test failed when running it all by itself. It was running the tests in a random order which helps to find irregular issues such as this.

Bug hunting!

For some reason when clicking on one of the items, it shows all of the results but when I have scripting issue a click event on a label, results don’t show.

I’ve removed all of the other tests to confirm that the problem exists separate from them. When adding back the other tests, it is only the sandbox tests that result in the clearAll test passing. What is the sandbox test doing that’s affecting the clearAll test?

This seems crazy, but when the describe section for the sandbox test is completely deleted the clearAll test still continues to work, but not when the empty describe section is removed. What this means is that I can have only the searchresults text going, and by adding an empty describe section above it, that causes the clearAll test to pass.

describe("Sandbox", function () {
});
describe("Search results", function () {
    ...
});

That’s just nuts! The code can’t remain that way, and the extra describe section is removed, so that I can carry on hunting down the cause of the problem.

After trying out some different things and ending up at dead ends, I notice that some console.log commands show that the unit test is running before checkboxlist has added the click event. No wonder the click event isn’t happening when the test clicks on the checkbox item.

I don’t want to mess around too much with how the tests work, and the searchResults code doesn’t need to wait for the DOM to be ready. Removing the jQuery wrapper and replacing it with standard iife code, results in the test now happening in the correct order.

// jQuery(function ($) {
(function iife($) {
    ...
// });
}(jQuery));

After lots of further testing a solution is finally achieved.

  • the sandbox doesn’t need to use jQuery to add the items
  • the additional describe section isn’t needed
  • the jQuery wrapper does need to be removed from the searchResults and filtercheckboxesbytext code, so that the tests can run afterwards instead of before.
  • and, having the test use an input field instead of a label field resulted in final success.
        var item = categories.querySelector("label");
        var item = categories.querySelector("input");

In actual usage when you click on the label, the for-attribute results in the proper input being clicked as well, so no problems will occur there.
I’m still not happy though about needing to remove the jQuery wrapper, and that clicking a test item results in all results wrongly showing.

The jQuery wrapper is used on both the searchResults code and the filtercheckboxesbytext code. Neither of them need to be in the jQuery wrapper, so removing that wrapper results in working tests, and the normal code still continues to work.

Any code inside of the jQuery wrapper cannot be tested, so if at some later stage we realize that’s needed, we can have the code in a separate testable object, and use the jQuery wrapper to then later on call that code. That’s not needed now though, so progress can occur.

Fixing the number of shown results

Now that the clearAll test is properly working, we can find out what’s causing all of the results to show when any single category item is clicked.

At some earlier time I thought that showing the results section would help:

    function showResults(results, targetSelector) {
        if (results.length > 0) {
            ...
            $(resultsSelector).show();
        }
    }

But that only ends up showing everything in the results area instead. I must add a test to catch this error, before fixing up the code. The problem is that the results section is hidden when it doesn’t have to be.

<div id="results" style="display:none">...</div>

Instead of hiding the results, I want everything inside of it to be hidden instead. So first, we test for the problem.

        it("hides only the result contents, not the whole results section itself", function () {
            resultsController.init(opts);
            queue.remove("Fruits");
            resultsController.updateResults(queue);
            expect($("#results").is(":visible")).toBe(true);
        });

This test has helped me to notice that the resultsSelector had the wrong value:

            // resultsSelector: "#results",
            resultsSelector: ".mydivs1",

I do plan to improve that resultsSelector piece at some stage to instead do its thing using the results container instead.

All of the tests now work, and the search results code didn’t have to be updated for this fix. I’ll keep the test in place though, to help protect against regression back to the problem when doing further updates.

That’s good progress for now. Fixing these bugs is something that might not have ever happened without the tests. With them in place, I was assured that everything else with the code remained working properly, allowing me to deeply explore the problem and come up with a working solution with as few changes as possible.

Next I’ll be working on the doAfter functions and the submit and click event parts of the code.

3 Likes

After running the tests from the last post, I find that they sometimes pass and sometimes don’t. I have the tests running at random to help find these strange situations, and have edited the Jasmine boot.js file so that it defaults to random testing.

Random tests by default

Normally Jasmine doesn’t do random tests and instead lets you turn them on in the Options section of the test runner.
I’ve made a small change to the Jasmine boot.js file, so that it defaults to doing random tests instead.

  // var random = queryString.getParam("random");
  var random = queryString.getParam("random") === undefined
    ? true
    : queryString.getParam("random");

You can download the files and the code as it currently is, from https://1drv.ms/u/s!ApgQG07VOr4pgsZMdwAKAl9PxgRfZw

What fails?

Frequently the random tests pass, but sometimes they fail and when we click on the Spec List, we can see that a few of the Search results tests are failing. There are other times that they don’t fail though. This means that something earlier before them is causing a side-effect that’s interfering with the test.

Reordering the tests to always have the failure is done next. Fortunately I don’t need to reorder things too much as turning off random testing always guarantees that the relevant tests will fail. I don’t want to change those failing tests. Instead, I want to find out what is interfering with those tests and fix things elsewhere so that the side-effects causing the failing tests no longer occur.

What’s causing the problem?

We can removing all of the other spec tests from the SpecRunner.html results to confirm if any of them are causing the problem.

<!--<script src="sandbox.spec.js"></script>
    <script src="queue.spec.js"></script>
    <script src="scroll.spec.js"></script>
    <script src="scrollmanager.spec.js"></script>
    <script src="filtercheckboxesbytext.spec.js"></script>
    <script src="checkboxlist.spec.js"></script>
    <script src="resultscontroller.spec.js"></script>-->
    <script src="searchresults.spec.js"></script>

The Search results tests now pass, so one (or more) of the other tests is causing the problem.

The scrollmanager tests are causing a problem with the searchresults tests, and the checkboxlist tests are causing another problem.

    <!-- <script src="sandbox.spec.js"></script> -->
    <!-- <script src="queue.spec.js"></script> -->
    <!-- <script src="scroll.spec.js"></script> -->
    <script src="scrollmanager.spec.js"></script>
    <!-- <script src="filtercheckboxesbytext.spec.js"></script> -->
    <script src="checkboxlist.spec.js"></script>
    <!-- <script src="resultscontroller.spec.js"></script> -->
    <script src="searchresults.spec.js"></script>

Scrollmanager side-effects

What are the scrollmanager tests doing that’s interfering with the searchresults tests?

    <!-- <script src="sandbox.spec.js"></script> -->
    <!-- <script src="queue.spec.js"></script> -->
    <!-- <script src="scroll.spec.js"></script> -->
    <script src="scrollmanager.spec.js"></script>
    <!-- <script src="filtercheckboxesbytext.spec.js"></script> -->
    <!-- <script src="checkboxlist.spec.js"></script> -->
    <!-- <script src="resultscontroller.spec.js"></script> -->
    <script src="searchresults.spec.js"></script>

The two searchresults tests that fail have two sets of clicks that occur in them.

    it("hide the clearAll button when no more results are shown", function () {
        var item = categories.querySelector("input");
        item.click();
        item.click();
        expect(isVisible(clearAllButton)).toBe(false);
    });
    ...
    it("hides a result when a category is selected then deselected", function () {
        var item = categories.querySelector("#chkFruits");
        $(item).click();
        $(item).click();
        expect(hasVisibleResults()).toBe(false);
    });

What’s going wrong when the scrollmanager tests are active? We can log out the button visibility before and after each click to find out.

        console.log(item, isVisible(clearAllButton));
        item.click();
        console.log(item, isVisible(clearAllButton));
        item.click();
        console.log(item, isVisible(clearAllButton));

When the tests are working I see:

false
true
false

And when the scrollmanager tests are active, I see:

false
true
true

So for some reason, clicking on an active button doesn’t make it inactive again. And yet, when I manually click on Fruits or Vegetables, they show and hide their results without any problem.

Using the debugger before the first click helps to reveal more about what’s causing the problem.

        debugger;
        console.log(item, isVisible(clearAllButton));
        item.click();

That helps me to see that when the Fruit item is clicked, that both the Fruit and the Vegetable results are shown. The second click removes the Fruit result, leaving the Vegetables and failing the test.

What’s causing both results to show when only one of them is clicked?

Finding the cause

I can move the debugger statement to the scrollmanager code:

    function clickHandler(evt, doAfterClick) {
        debugger;
        ...
    }

And I can add a console.log to the beforeEach section of the searchresults tests, to ensure that I know when that section of tests is starting.

    beforeEach(function () {
        console.log("search results tests");
        ...

Rerunning the tests has the debugger breaking in at the click events. I can press the resume button through the first two, after which “search results tests” appears in the console letting me know I’m in the right place, and when stepping over the code, the doAfterClick() function causes both the Fruit and Vegetable sets of results to appear, when only Fruit is desired. We’re getting closer.

Following the code execution through into the doAfterClick() function (which is initialized in searchresults.js), I find that the results variable already contains both Fruits and Vegetables. It’s because they aren’t being removed which is causing the problem.

        doAfterClick: function updateResults(checkbox) {
            var value = checkbox.value;
            if ($(checkbox).is(":checked")) {
                results.add(value);
            } else {
                results.remove(value);
            }
            resultsController.updateResults(results);
            toggleClearAllBasedOnResults(results);
        }

I just need to tell the scrollmanager tests to remove those queue results.

Removing the queue results

We can do that by clicking on the items a second time so that they’re removed from the queue, so that it’s nice and empty when other tests occur.

    it("selects the clicked category item", function () {
        ...
        expect(document.activeElement).toBe(secondItem);
        firstItem.click();
        secondItem.click();
    });

The searchresults tests now pass when the scrollmanager tests are active.

Checkboxlist side-effects

The checkboxlist tests are likely to have similar side effects. I can find out which test is causing the problem by renaming it to xit to temporarily disable them. It’s only the one test that’s causing the problem:

    it("checks the first available checkbox", function () {
        initCheckboxList();
        $("p:eq(0)", categories).hide();
        $("p:eq(1)", categories).hide();
        expect($("input:eq(0)", categories).prop("checked")).toBe(false);
        expect($("input:eq(1)", categories).prop("checked")).toBe(false);
        expect($("input:eq(2)", categories).prop("checked")).toBe(false);
        checkboxList.checkFirst();
        expect($("input:eq(0)", categories).prop("checked")).toBe(false);
        expect($("input:eq(1)", categories).prop("checked")).toBe(false);
        expect($("input:eq(2)", categories).prop("checked")).toBe(true);
        checkboxList.checkFirst();
        $("p:eq(0)", categories).show();
        $("p:eq(1)", categories).show();
    });

The last three lines after expect were an attempt to undo what the test had done, which clearly doesn’t work.
Instead, we can use the uncheckByValue() function to undo what checkFirst did.

        expect($("input:eq(2)", categories).prop("checked")).toBe(true);
        // checkboxList.checkFirst();
        checkboxList.uncheckByValue($("input:eq(2)", categories).attr("id"));
        // $("p:eq(0)", categories).show();
        // $("p:eq(1)", categories).show();

Those other two lines aren’t needed either.

The tests now fully pass, and the console.log lines can be removed from the searchresults code:

    it("hide the clearAll button when no more results are shown", function () {
        var item = categories.querySelector("input");
        // console.log(item, isVisible(clearAllButton));
        item.click();
        // console.log(item, isVisible(clearAllButton));
        item.click();
        // console.log(item, isVisible(clearAllButton));
        expect(isVisible(clearAllButton)).toBe(false);
    });

And, the SpecRunner.html spec scripts can all be brought back again:

    <script src="sandbox.spec.js"></script>
    <script src="queue.spec.js"></script>
    <script src="scroll.spec.js"></script>
    <script src="scrollmanager.spec.js"></script>
    <script src="filtercheckboxesbytext.spec.js"></script>
    <script src="checkboxlist.spec.js"></script>
    <script src="resultscontroller.spec.js"></script>
    <script src="searchresults.spec.js"></script>

Random tests

Running the tests in random order should now reveal no more problems, but there is a problem that reveals itself. When the checkboxlist tests are run in a different order, other tests in there end up causing other problems with the Queue.

I don’t want to have to clean up after each test. It would be much better if cleaning up was done automatically after each test instead, which is what the afterEach() function is used for. I can loop through each of the category values and uncheck them before moving on to the next test.

    afterEach(function () {
        $("input", categories).each(function (index, input) {
            checkboxList.uncheckByValue($(input).val());
        });
    });

With that in place in the checkboxlist() test, no more errors occur, and I can even remove the code that I added before:

        expect($("input:eq(2)", categories).prop("checked")).toBe(true);
        // checkboxList.uncheckByValue($("input:eq(2)", categories).attr("id"));

and when running multiple tests in a random order, everything continue to work perfectly well.

That will do for this set of testing. I’ve added some empty tests to act as a reminder when I come back to this next week.

The code as it currently stands can be obtained from https://1drv.ms/u/s!ApgQG07VOr4pgsZOFPp_aG7vklmIYA

3 Likes

Cleaning up from last time

One of the tests still has console log commands, so we need to remove those before carrying on.

    it("hide the clearAll button when no more results are shown", function () {
        var item = categories.querySelector("input");
        // console.log(item, isVisible(clearAllButton));
        item.click();
        // console.log(item, isVisible(clearAllButton));
        item.click();
        // console.log(item, isVisible(clearAllButton));
        expect(isVisible(clearAllButton)).toBe(false);
    });

Testing click events

I was going to test that the click event occurs by using a spyOn technique, but I’m already testing that it works by checking for beneficial side-effects in the “shows a result when a category is selected” test, so I don’t need the click event test at all and that can be completely removed.

The event handler problem

With the Clearall button, it works on the base HTML but not on testing HTML. We have two options here. Currently we use a variety of techniques to listen to events happening on the page. We have the whole page watching for any valid interaction, and we have individual elements being assigned event handlers as well.

Having the whole page listen for events is less friendly and more likely to result in conflicts with other things. Instead of having the whole page listen for events, it makes more sense to pass the categories and results containers to an init function, so that control can be delegated from there instead.

As a result, all parts of the code that use a global event listener, need to be modified so that it uses a more local event listener instead.

Fortunately we have tests for most of the code, so it should be relatively easy to make these changes. What parts need to be considered?

  • checkboxlist: $(document).on(“click”, “:checkbox”, clickHandler);
  • resultcontroller: $(closeButtonSelector).on(“click”, closeClickWrapper(opts.doAfterClose));
  • searchresults: $(searchForm).on(“submit”, formSubmitHandler);
  • searchresults: $(clearAllButton).on(“click”, clearAllClickHandler);

Fixing Checkboxlist event handler

Checkboxlist places the event listener on the whole document, when it should instead only be on the categories area itself.
Because the event listener will no longer be on the whole document, we’ll need to add them again for other tests.

Making the following change causes several tests to fail:

        // $(document).on("click", ":checkbox", clickHandler);
        $(container).on("click", ":checkbox", clickHandler);

because those tests need to have the event listener added to them.

To achieve this, we can make searchResults return an object that contains the init code.

var searchResults = (function ($) {
    ...
    return {
        init: function () {
            var results = Queue.init();
            ...
            $(searchForm).on("submit", formSubmitHandler);
            $(clearAllButton).on("click", clearAllClickHandler);
        }
    };
}(jQuery));
searchResults.init();

That last line gets it working on the base code, and for the test code we can add that last init line to the beforeEach section too.

scrollmananger.spec.js

    beforeEach(function () {
        ...
        searchResults.init();
    });

searchresults.spec.js

    beforeEach(function () {
        ...
        searchResults.init();
    });

And, all of the tests return back to fully passing.

Fixing resultsController

In the resultscontroller tests, all of the tests call the init function, so that really should be moved up to the beforeEach function.

    beforeEach(function () {
        ...
        resultsController.init(opts);
    });
    it("initializes a doAfterClose function", function () {
        spyOn(opts, "doAfterClose");
        // resultsController.init(opts);
        ...
    });
    describe("updates the results", function () {
        it("shows an item when there was none before", function () {
            // resultsController.init(opts);
            ...
        });
        it("removes an item when it was there before", function () {
            // resultsController.init(opts);
            ...
        });
        it("shows nothing when there's nothing to show", function () {
            // resultsController.init(opts);
            ...
        });
        it("shows all items when the update changes nothing", function () {
            // resultsController.init(opts);
            ...
        });
        it("hides only the result contents, not the whole results section itself", function () {
            // resultsController.init(opts);
            ...
        });
    });
    it("can remove an item from the queue", function () {
            // resultsController.init(opts);
            ...
    });

All of the tests still pass, except for the doAfterClose function which needs to be improved.

    it("initializes a doAfterClose function", function () {
        spyOn(opts, "doAfterClose");
        resultsController.init(opts);
        $(".close:first").click();
        expect(opts.doAfterClose).toHaveBeenCalled();
    });

It’s possible to move the spying into the beforeEach function, but only one of the tests uses it so that doesn’t seem to be a good idea.
Instead, the spyOn can be removed and replaced with checking if the result was removed.

    beforeEach(function () {
        ...
            doAfterClose: function (name) {
                $("#" + name).hide();
            }
        ...
    });
    it("initializes a doAfterClose function", function () {
        $(".mydivs1:first").show();
        expect($(".mydivs1:visible").length).toBe(1);
        $(".mydivs1:first .close", "#results").click();
        expect($(".mydivs1:visible").length).toBe(0);
    });

Fixing doAfterClose

The doAfterClose function shouldn’t deal with the main task of closing a result. Instead it should be for additional things if needed after closing the result.

resultscontroller.spec.js

        opts = {
            resultsSelector: ".mydivs1",
            closeButtonSelector: ".close",
            queue: queue
            // queue: queue,
            // doAfterClose: function (name) {
            //     $("#" + name).hide();
            // }
        };

The tests all go back to working now, which helps to demonstrate that the doAfterClose function shouldn’t be responsible for hiding the results.

Fixing past sins

Here’s a summary of the main events that cross over to other areas:

checkboxlist-click -> searchresults-doafterclick
checkboxlist-keydown -> scrollmanager-keydownhandler
resultscontroller-clickclose -> searchresults-doafterclose -> checkboxList-uncheckByValue
search-clearall -> category-click, result-click

Preventing higher-level requests

Currently all of the base code has been loaded while putting the tests together, which has resulted in some bad habits being allowed to remain.

Improving scrollmanager tests

Removing filtercheckboxesbytext, checkboxlist, resultscontroller, and searchresults from the base results and the tests, reveals the first problem. Scrollmanager requests for information from the searchresults code.

It’s the “selects the clicked category item” test that causes this problem, and on reflection I realize that this test doesn’t belong in the scrollmanager section. Clicking on results move correctly belongs with the checkboxlist tests instead.

So, the searchResults init can be removed, along with the test that doesn’t belong with the scrollmanager tests.

    beforeEach(function () {
        ...
        // searchResults.init();
    });
    ...
    // it("selects the clicked category item", function () {
    //     var firstItem = items[0].querySelector("input");
    //     var secondItem = items[1].querySelector("input");
    //     firstItem.click();
    //     expect(document.activeElement).toBe(firstItem);
    //     secondItem.click();
    //     expect(document.activeElement).toBe(secondItem);
    // });

The scrollmanager tests all now pass without requiring other code, so we can move on to the next set of code.
The filtercheckboxesbytext code when enabled passes without issue, so it’s on to checkboxlist.

Improving checkboxlist tests

Enabling the checkboxlist code and tests shows everything all passes, and that test removed from scrollmananger can now be added in a slightly modified way too.

    it("selects the clicked category item", function () {
        initCheckboxList();
        var firstItem = $("input:eq(0)", categories).get(0);
        var secondItem = $("input:eq(1)", categories).get(0);
        firstItem.click();
        expect(document.activeElement).toBe(firstItem);
        secondItem.click();
        expect(document.activeElement).toBe(secondItem);
    });

I also see in checkboxlist that a lot of the tests first call the init function. If I can get rid of the spying I can then simplify things and move the init into the beforeEach section.

I can first copy the init function code into the beforeEach section:

    beforeEach(function () {
        ...
        checkboxList.init({
            container: categories,
            doAfterClick: handlers.doAfterClick
        });
    });

The tests still call the init function, but all of the tests still work while I clean things up.
The init code can now be removed from many of the tests where spying doesn’t occur:

    it("selects the clicked category item", function () {
        // initCheckboxList();
        ...
    });
    ...
    it("checks the first checkbox", function () {
        // initCheckboxList();
        ...
    });
    it("checks the first available checkbox", function () {
        // initCheckboxList();
        ...
    });
    it("doesn't uncheck a checkbox that's already already checked", function () {
        // initCheckboxList();
        ...
    });
    it("unchecks a checkbox by finding its value property", function () {
        // initCheckboxList();
        ...
    });

And that just leaves the spying to deal with. The checkboxlist doesn’t need to care if clicking things has an effect or not. That’s for other code such as the results controller to care about, so the spying tests can all be removed.

    // it("executes doAfterClick when an input is clicked", function () {
    //     spyOn(opts, "doAfterClick");
    //     checkboxList.init(opts);
    //     expect(opts.doAfterClick).not.toHaveBeenCalled();
    //     categories.querySelector("input").click();
    //     expect(opts.doAfterClick).toHaveBeenCalled();
    // });
    // it("calls the doAfterClick handler when a checkbox is clicked", function () {
    //     spyOn(handlers, "doAfterClick");
    //     initCheckboxList();
    //     expect(handlers.doAfterClick).not.toHaveBeenCalled();
    //     $(":checkbox:first", categories).click();
    //     expect(handlers.doAfterClick).toHaveBeenCalled();
    // });
    // it("calls the keydownHandler on a key click", function () {
    //     spyOn(scrollManager, "keydownHandler");
    //     initCheckboxList();
    //     expect(scrollManager.keydownHandler).not.toHaveBeenCalled();
    //     keyDown(categories, " ");
    //     expect(scrollManager.keydownHandler).toHaveBeenCalled();
    // });

The init function can now be removed from the test,

    // function initCheckboxList() {
    //     checkboxList.init({
    //         container: categories,
    //         doAfterClick: handlers.doAfterClick
    //     });
    // }

and everything carries on working while being better organised.

Considering the main events

The rest of the test code seems to be fine too, so we can return to considering the main idea that we want to work on.

These are the main events again:

checkboxlist-click -> searchresults-doafterclick
checkboxlist-keydown -> scrollmanager-keydownhandler
resultscontroller-clickclose -> searchresults-doafterclose -> checkboxList-uncheckByValue
search-clearall -> category-click, result-click

Some of them get information from higher-level sections, such as checkboxlist-click and resultscontroller-clickclose, and we need to stop them doing that. The code should only know about things in it’s own area, and shouldn’t reach out to higher-levels for things.

I need to stop them reaching out like that, and consider if afterClick methods really are a good solution to what’s happening here - which I’ll have a think about for next time.

3 Likes

When the scrollmanager code prompted testing for the click events, that’s prompted me to understand that the click handler doesn’t belong there.

Before we get going

While doing the below updates I learned that one of the tests had a syntax error resulting in all of the resultscontroller tests failing to even appear in the test results. As a result, I’ve undone all of the code changes that I was working on, so that we can get the tests working first before making further changes to the code.

One of the tests has an accidental hash symbol instead of a dollar symbol, that results in a syntax error preventing all of those tests from running.

resultscontroller.spec.js

        // #(".mydivs1:first").show();
        $(".mydivs1:first").show();

With that corrected, the resultscontroller tests all appear and one of them fails. Comparing how the test inits the resultsController, I can see that the test didn’t include the doAfterClose information, which we should add.

resultscontroller.spec.js

        opts = {
            resultsSelector: ".mydivs1",
            closeButtonSelector: ".close",
            queue: queue,
            doAfterClose: checkboxList.uncheckByValue
        };

And with that the tests pass. It’s not good though that seemingly optional information such as doAfterClose results in everything failing. The resultscontroller shouldn’t use an optional event to perform its basic functionality. As we have no real benefit from doAfterClose, it really should be removed.

Also when a result is clicked, the searchresults section adds a doAfterClick method that uses a checkboxlist-uncheckbyvalue method. That seems to be too much work, but we’ll see how things go with that after doing some cleaning up.

Tidying up the results click handler

How does resultscontroller close one of the results?
- $(closeButtonSelector).on("click", closeClickWrapper(opts.doAfterClose));

When we comment out the doAfterClose, we should expect that the results close but the checkbox item remains selected.
However, when we do that we find that the doAfterClose function is responsible for closing the results item as well as the checkboxlist item, which doesn’t seem to be a good structure.

Currently the call stack when closing a result is quite complex:

  • jquery-dispatch event (user clicks on the close button)
  • resultscontroller-closeclickhandler
  • checkboxlist-uncheckbyvalue
  • jquery-dispatch (trigger click on the checkbox item)
  • checkboxlist-clickhandler
  • searchresults-updateresults
  • resultscontroller-updateresults

So effectively, the close button doesn’t actually do anything, and the doAfterClose method ends up clicking the checkbox item so that it can do all of the work.

While it is possible to re-engineer the close event so that it closes the result before dealing with the checkbox item, I think that a better mid-term goal is to remove the doAfterClose event so that the init function doesn’t need to configure that behaviour.

We can replace doAfterClose in the results controller with the call to uncheckByValue

resultscontroller.js

    function closeClickHandler(evt, doAfterClose) {
        var button = evt.target;
        var itemName = $(button).parents(resultsSelector).attr("id");
        // doAfterClose(itemName);
        checkboxList.uncheckByValue(itemName);
    }

The tests still pass, and we can now remove the doAfterClose information from resultscontroller, searchresults, and the resultscontroller test

resultscontroller.js

    // function closeClickHandler(evt, doAfterClose) {
    function closeClickHandler(evt) {

searchresults.js

            resultsController.init({
                ...
                // doAfterClose: checkboxList.uncheckByValue
            });

resultscontroller.spec.js

        opts = {
            ...
            // doAfterClose: checkboxList.uncheckByValue
        };

And now, the event wrapper is no longer needed, allowing us to remove that as well.

resultscontroller.js

    // function closeClickWrapper(doAfterClose) {
    //     return function wrappedCloseClick(evt) {
    //         return closeClickHandler(evt, doAfterClose);
    //     };
    // }
    function init(opts) {
        ...
        // $(closeButtonSelector).on("click", closeClickWrapper(opts.doAfterClose));
        $(closeButtonSelector).on("click", closeClickHandler);
    }

Tidying up the checkbox click handler

This is also a good time to look at the other event handlers. The scrollmanager code currently handles the click event, which is much more suited to be handled by the checkboxlist code instead.
As the clickHandler is moving out of the scrollmanager, we need to make accessible the moveTo method as well.

scrollmanager.js

    // function clickHandler(evt, doAfterClick) {
    //     var el = evt.target;
    //     moveTo($(el));
    //     doAfterClick(el);
    // }
    // function clickWrapper(doAfterClick) {
    //     return function wrappedClickHandler(evt) {
    //         clickHandler(evt, doAfterClick);
    //     };
    // }
    ...
    return {
        // clickWrapper: clickWrapper,
        keydownHandler: keydownHandler,
        moveTo: moveTo
    };

The clickHandler that we move into checkboxlist only needs to have the moveTo function updated so that it requests it from scrollmanager.

checkboxlist.js

    function clickHandler(evt, doAfterClick) {
        var el = evt.target;
        scrollManager.moveTo($(el));
        doAfterClick(el);
    }
    function clickWrapper(doAfterClick) {
        return function wrappedClickHandler(evt) {
            clickHandler(evt, doAfterClick);
        };
    }
    function init(opts) {
        // var clickHandler = scrollManager.clickWrapper(opts.doAfterClick);
        var clickHandler = clickWrapper(opts.doAfterClick);
        ...
    }

So now checkboxlist is moved to the appropriate category, and uses doAfterClick to close any related results.

Improving doAfterClick

The searchresults init function shouldn’t define what occurs when things are clicked. That behaviour needs to be moved so that it’s more specific to the area that it affects.

Here’s the section of code that we’ll be improving:

searchresults.js

                doAfterClick: function updateResults(checkbox) {
                    var value = checkbox.value;
                    if ($(checkbox).is(":checked")) {
                        results.add(value);
                    } else {
                        results.remove(value);
                    }
                    resultsController.updateResults(results);
                    toggleClearAllBasedOnResults(results);
                }

All of that doAfterClick code needs to be pushed into the resultscontroller code.

That resultsController.updateResults part near the end of the above excerpt, is not really updating the results. Instead it’s refreshing the results, so we can rename it to refreshResults.

searchresults.js

                    // resultsController.updateResults(results);
                    resultsController.refreshResults(results);

resultscontroller.js

    // function updateResults(queue) {
    function refreshResults(queue) {
        ...
    }
    ...
    return {
        ...
        // updateResults: updateResults,
        refreshResults: refreshResults,

This causes the tests to break. Looking at the ones that break based on the change that was made, helps us to see that those tests are too specific and need to be done in a more general manner.

First we get the tests working again, which also gives good opportunity to consider a choice that was made to start the queue with a value, which for clarity works better when the queue starts off as empty.

resultscontroller.spec.js

    beforeEach(function () {
        queueItems = [];
        ...
    });
    ...
        it("shows an item when there was none before", function () {
            resultsController.refreshResults(queue);
            expect(hasVisibleResults()).toBe(false);
            queueItems = ["Fruits"];
            resultsController.refreshResults(queue);
            expect(hasVisibleResults()).toBe(true);
        });
        it("removes an item when it was there before", function () {
            queueItems = ["Fruits"];
            resultsController.refreshResults(queue);
            expect(hasVisibleResults()).toBe(true);
            queueItems = [];
            resultsController.refreshResults(queue);
            expect(hasVisibleResults()).toBe(false);
        });
        it("shows nothing when there's nothing to show", function () {
            resultsController.refreshResults(queue);
            expect(hasVisibleResults()).toBe(false);
            resultsController.refreshResults(queue);
            expect(hasVisibleResults()).toBe(false);
        });
        it("shows all items when the update changes nothing", function () {
            queueItems = ["Fruits"];
            resultsController.refreshResults(queue);
            expect(hasVisibleResults()).toBe(true);
            resultsController.refreshResults(queue);
            expect(hasVisibleResults()).toBe(true);
        });

And the tests return to passing without error.

Moving updateResults code into resultscontroller

Instead of moving the code all at once when things are likely to break (as it has done on previous attempts), we can move the doAfterClick code from searchresults over to resultscontroller a bit at a time.

We can start by creating an empty updateResult() function in the results controller, and calling that function from the searchresults code.

resultscontroller.js

    function updateResult(checkbox, results) {
    }
    ...
    return {
        ...
        updateResult: updateResult,
        ...
    };

searchresults.js

                doAfterClick: function updateResults(checkbox) {
                    var value = checkbox.value;
                    if ($(checkbox).is(":checked")) {
                        results.add(value);
                    } else {
                        results.remove(value);
                    }
                    resultsController.updateResult(checkbox, results);
                    resultsController.refreshResults(results);
                    toggleClearAllBasedOnResults(results);
                }

Now that a place exists in resultscontroller for updateResult, we need to make sure that we have tests to ensure that the code we plan to put in there, correctly does its job.

The searchresults tests have plenty of click-based tests that make sure that things work properly.

The code can now be moved from the doAfterClick function into the updateResult function instead.

searchresults.js

                doAfterClick: function updateResults(checkbox) {
                    // var value = checkbox.value;
                    // if ($(checkbox).is(":checked")) {
                    //     results.add(value);
                    // } else {
                    //     results.remove(value);
                    // }
                    resultsController.updateResult(checkbox, results);
                    // resultsController.refreshResults(results);
                    toggleClearAllBasedOnResults(results);
                }

resultscontroller.js

    function updateResult(checkbox, results) {
        var value = checkbox.value;
        if ($(checkbox).is(":checked")) {
            results.add(value);
        } else {
            results.remove(value);
        }
        // resultsController.updateResult(checkbox, results);
        // resultsController.refreshResults(results);
        refreshResults(results);
    }

Because we’re going to be using add and remove for the queue and there’s no overhead from using the queue, it makes sense to remove the faked queue from the resultscontroller test code, and use the proper queue instead.

resultscontroller.spec.js

    // var queueItems;
    var queue;
    beforeEach(function () {
        // queueItems = [];
        // queue = {
        //     getAll: function () {
        //         return queueItems;
        //     },
        //     remove: function (item) {
        //         queueItems = queueItems.filter(function (queueItem) {
        //             return queueItem !== item;
        //         });
        //     }
        // };
        queue = Queue.init();
        ...
    });

The tests can now be modified so that instead of queueItems, that the item is added or removed from the queue.

For example:

        it("removes an item when it was there before", function () {
            // queueItems = ["Fruits"];
            queue.add("Fruits");
            resultsController.refreshResults(queue);
            expect(hasVisibleResults()).toBe(true);
            // queueItems = [];
            queue.remove("Fruits");
            resultsController.refreshResults(queue);
            expect(hasVisibleResults()).toBe(false);
        });
        ...
    it("can remove an item from the queue", function () {
        // var itemToRemove = "Item to remove";
        // queueItems = ["Fruits", itemToRemove];
        queue.add("Fruits");
        queue.add("Item to remove");
        resultsController.removeItem("Item to remove");
        // expect(queueItems).toEqual(["Fruits"]);
        expect(queue.getAll()).toEqual(["Fruits"]);
    });

Here is the updated searchresults code:

            checkboxList.init({
                container: $(categories),
                doAfterClick: function updateResults(checkbox) {
                    resultsController.updateResult(checkbox, results);
                    toggleClearAllBasedOnResults(results);
                }
            });

After a checkbox item is clicked, it updates the result and then sets the visiblity of the clearAll button, which makes much more sense.

All of the above changes can look to be quite scary, but the tests are there to help reassure us that everything keeps on working as expected. With the tests helping to lessen the fear that “change might break things”, it’s easier for good progress to occur.

The init commands are looking much better now. Next up I’ll revisit the tests yet to be done in the searchresults code. When that’s done I can then take a second look through the test code, looking for improvements to make, before considering the testing as complete.

3 Likes

The init commands are looking much better now. What gets done with the code today is:

  • a separation of concerns
  • using a better function name
  • remove the resultscontroller queue
  • remove the resultscontroller tests queue
  • complete the resultscontroller tests
  • complete the searchresults tests
  • test the form submit

A separation of concerns

On reflection, the queue information doesn’t belong in the resultscontroller. The results controller should only deal with the results, and not with managing the queue.

Here’s the queue-based existing code in resultscontroller that needs to be improved:

resultscontroller.js

    function refreshResults(queue) {
        clearResults();
        showResults(queue.getAll(), "#results");
    }
    function updateResult(checkbox, results) {
        var value = checkbox.value;
        if ($(checkbox).is(":checked")) {
            results.add(value);
        } else {
            results.remove(value);
        }
        refreshResults(results);
    }

The updateResults function doesn’t belong in resultscontroller and needs to be moved to searchresults.

resultscontroller.js

    // function updateResult(checkbox, results) {
    //     var value = checkbox.value;
    //     if ($(checkbox).is(":checked")) {
    //         results.add(value);
    //     } else {
    //         results.remove(value);
    //     }
    //     refreshResults(results);
    // }
    return {
        ...
        // updateResult: updateResult,
    };

searchresults.js

    return {
        init: function init() {
            var results = Queue.init();
            function updateResult(checkbox, results) {
                var value = checkbox.value;
                if ($(checkbox).is(":checked")) {
                    results.add(value);
                } else {
                    results.remove(value);
                }
                resultsController.refreshResults(results);
            }
            ...
                    // resultsController.updateResult(checkbox, results);
                    updateResult(checkbox, results);

The tests still work, and updateResults can be moved to a better place in resultscontroller.

Moving updateResult up to where the other functions are, means that the results object needs to be moved too.

searchresults.js

    var results = {};
    ...
    function toggleClearAllBasedOnResults(results) {
        ...
    }
    function updateResult(checkbox, results) {
        var value = checkbox.value;
        if ($(checkbox).is(":checked")) {
            results.add(value);
        } else {
            results.remove(value);
        }
        resultsController.refreshResults(results);
    }
    function clearAllClickHandler() {
        ...
    }
    ...
        init: function init() {
            var results = Queue.init();
            results = Queue.init();
            // function updateResult(checkbox, results) {
            //     var value = checkbox.value;
            //     if ($(checkbox).is(":checked")) {
            //         results.add(value);
            //     } else {
            //         results.remove(value);
            //     }
            //     resultsController.refreshResults(results);
            // }

We can now turn our attention to resultscontroller code. the refreshResults shouldn’t know any details about how the queue works, so instead, the searchresults can give it an array of items to show.

searchresults.js

    function updateResult(checkbox, results) {
        ...
        // resultsController.refreshResults(results);
        resultsController.refreshResults(results.getAll());
    }

resultscontroller.js

    // function refreshResults(results) {
    function refreshResults(items) {
        clearResults();
        // showResults(results.getAll(), "#results");
        showResults(items, "#results");
    }

This breaks some tests, which can be fixed, but we should also fix something else.

Using a better function name

While the refreshResults name seems to be better than what it had before, refreshing is something that doesn’t normally accept a list of items to update. It’s also not feasible to have refreshResults reach out and get the queue items because the resultscontroller shouldn’t be worried about dealing with that, so renaming refreshResults to updateResults is a better solution. This is a change to the API, which is how other code interacts with the resultscontroller, so other changes will be needed too.

resultscontroller.js

    // function refreshResults(results) {
    function updateResults(items) {
        clearResults();
        // showResults(results.getAll(), "#results");
        showResults(items, "#results");
    }
    ...
    return {
        // refreshResults: refreshResults,
        updateResults: updateResults,
        ...
    };

Because the results controller API changed, the searchresults code and the resultscontroller tests need to be updated too so that they continue to work.

searchresults.js

        // resultsController.refreshResults(results.getAll());
        resultsController.updateResults(results.getAll());

resultscontroller.spec.js

        it("shows an item when there was none before", function () {
            // resultsController.refreshResults(queue);
            resultsController.updateResults(queue.getAll());
            expect(hasVisibleResults()).toBe(false);
            queue.add("Fruits");
            // resultsController.refreshResults(queue);
            resultsController.updateResults(queue.getAll());
            expect(hasVisibleResults()).toBe(true);
        });
        it("removes an item when it was there before", function () {
            queue.add("Fruits");
            // resultsController.refreshResults(queue);
            resultsController.updateResults(queue.getAll());
            expect(hasVisibleResults()).toBe(true);
            queue.remove("Fruits");
            // resultsController.refreshResults(queue);
            resultsController.updateResults(queue.getAll());
            // queueItems = [];
            expect(hasVisibleResults()).toBe(false);
        });
        it("shows nothing when there's nothing to show", function () {
            // resultsController.refreshResults(queue);
            resultsController.updateResults(queue.getAll());
            expect(hasVisibleResults()).toBe(false);
            // resultsController.refreshResults(queue);
            resultsController.updateResults(queue.getAll());
            expect(hasVisibleResults()).toBe(false);
        });
        it("shows all items when the update changes nothing", function () {
            queue.add("Fruits");
            // resultsController.refreshResults(queue);
            resultsController.updateResults(queue.getAll());
            expect(hasVisibleResults()).toBe(true);
            // resultsController.refreshResults(queue);
            resultsController.updateResults(queue.getAll());
            expect(hasVisibleResults()).toBe(true);
        });

That those tests failed is a good indicator of what occurs when the API between different systems are changed. If at all possible, try to make sure that refactoring the code doesn’t result in API changes, and do those API changes less frequently as a separate change to help reduce the amount of complexity that you have to deal with.

Removing the resultscontroller queue

Before updating the tests though, the last remanents of the queue should be removed from the resultscontroller code, by moving the removeItemFromQueue function out to the searchresults code.

resultscontroller.js

    // function removeItemFromQueue(item) {
    //     queue.remove(item);
    // }
    ...
    return {
        // removeItem: removeItemFromQueue,
        ...
    };

There’s nothing in the code that uses removeItem, so that can be removed from the tests as well.

resultscontroller.spec.js

    // it("can remove an item from the queue", function () {
    //     queue.add("Fruits");
    //     queue.add("Item to remove");
    //     resultsController.removeItem("Item to remove");
    //     expect(queue.getAll()).toEqual(["Fruits"]);
    // });

And we can finish tidying up the resultscontroller code by removing the queue.

resultscontroller.spec.js

    // var queue;
    ...
    function init(opts) {
        ...
        // queue = opts.queue;
        ...
    }

And that queue can now be removed from the init statements:

searchresults.js

            resultsController.init({
                // queue: results,
                ...

resultscontroller.spec.js

        opts = {
            ...
            // queue: queue
        };

Removing the resultscontroller tests queue

Now that resultscontroller no longer has the queue to worry about, we can return to the resultscontroller tests to improve them.

As the resultscontroller code no longer deals with the queue, we can remove the queue from resultscontroller tests, replacing them with an array instead.
For example, with the following test, we can remove the queue and replace it with an array.

resultscontroller.spec.js

        it("shows an item when there was none before", function () {
            // resultsController.updateResults(queue.getAll());
            resultsController.updateResults([]);
            expect(hasVisibleResults()).toBe(false);
            // queue.add("Fruits");
            resultsController.updateResults(["Fruits"]);
            expect(hasVisibleResults()).toBe(true);
        });

And the test carries on working, testing the same thing that it did before.

The same updates can be made to all of the other resultscontroller tests that use the queue.

resultscontroller.spec.js

        it("removes an item when it was there before", function () {
            // queue.add("Fruits");
            // resultsController.updateResults(queue.getAll());
            resultsController.updateResults(["Fruits"]);
            expect(hasVisibleResults()).toBe(true);
            // queue.remove("Fruits");
            // resultsController.updateResults(queue.getAll());
            resultsController.updateResults([]);
            expect(hasVisibleResults()).toBe(false);
        });
        it("shows nothing when there's nothing to show", function () {
            // resultsController.updateResults(queue.getAll());
            resultsController.updateResults([]);
            expect(hasVisibleResults()).toBe(false);
            // resultsController.updateResults([]);
            expect(hasVisibleResults()).toBe(false);
        });
        it("shows all items when the update changes nothing", function () {
            // queue.add("Fruits");
            // resultsController.updateResults(queue.getAll());
            resultsController.updateResults(["Fruits"]);
            resultsController.updateResults([]);
            expect(hasVisibleResults()).toBe(true);
            // resultsController.updateResults(queue.getAll());
            resultsController.updateResults(["Fruits"]);
            expect(hasVisibleResults()).toBe(true);
        });

The need for the queue has been completely removed from the resultscontroller tests, so the scaffolding for the queue can now be removed too.

resultscontroller.spec.js

    // var queue;
    beforeEach(function () {
        // queue = Queue.init();
        opts = {
            resultsSelector: ".mydivs1",
            // closeButtonSelector: ".close",
            // queue: queue
            closeButtonSelector: ".close"
        };

Completing the resultscontroller tests

We can now look at the resultscontroller tests to make sure that the tests fairly represent what the resultscontroller has to deal with, and gain full code coverage of that area.

First of all, looking at the code coverage report I see that the closeClickHandler function isn’t tested, so we need to have a test for that.

resultscontroller.spec.js

    it("closes a result when the close button is clicked", function () {
        resultsController.updateResults(["Fruits"]);
        expect(hasVisibleResults()).toBe(true);
        $("#Fruits .close", sandpit).click();
        expect(hasVisibleResults()).toBe(false);
    });

That should work, but currently the resultscontroller needs to use the checkbox item to close a result. Can we use this to help the resultscontroller to close the the result without needing to rely on the checkbox item? I think we can.

resultscontroller.js

    function closeClickHandler(evt) {
        var button = evt.target;
        // var itemName = $(button).parents(resultsSelector).attr("id");
        var id = $(button).parents(resultsSelector).attr("id");
        document.querySelector("#" + id).remove();
        // checkboxList.uncheckByValue(itemName);
        checkboxList.uncheckByValue(id);
    }

That results in tests working, and means that we can move the checkboxList statement out to searchresults. Before we move it out though, we need to provide easy access to the id of the result that was clicked.

We can also clean up a bit and remove the unused clearResults and showResults methods, as they are no longer accessed from the returned object.

resultscontroller.js

    function resultId(el, parentSelector) {
        return $(el).parents(parentSelector).attr("id");
    }
    ...
    function closeClickHandler(evt) {
        ...
        // var id = $(button).parents(resultsSelector).attr("id");
        var id = resultId(button, resultsSelector);
        ...
    }
    return {
        resultId: resultId,
        // clearResults: clearResults,
        // showResults: showResults,
        updateResults: updateResults,
        init: init
    };

The checkboxList line can now be removed from the resultscontroller click handler, and added instead to the searchresults code.

resultscontroller.js

    function closeClickHandler(evt) {
        ...
        // checkboxList.uncheckByValue(id);
    }

We can add another click handler from search results. The close button now has two different click events that occur which is perfectly fine, one to close the result and the other to uncheck the checkbox item.

searchresults.js

            $(closeButton).on("click", function (evt) {
                var value = resultsController.resultId(evt.target);
                checkboxList.uncheckByValue(value);
            });
            $(clearAllButton).on("click", clearAllClickHandler);

Completing the search results tests

The tests for search results has some incomplete tests. The following one is duplicate and can be removed.

searchresults.spec.js

    // it("updates the results when a category is clicked", function () {
    // });

That one was easy to deal with. The next test checks if the checkbox is unchecked when a result is closed.

searchresults.spec.js

    it("cleans up when closing by unclicking the categories", function () {
        
    });

We can comment out the checkboxList.uncheckByValue(value); line from searchresults.js to check if it’s tested by any other tests.

searchresults.js

            $(closeButton).on("click", function (evt) {
                var value = resultsController.resultId(evt.target);
                // checkboxList.uncheckByValue(value);
            });

The tests all still pass, so we need to create a test that fails when that commented-out line is not there, so that the test can pass when the code is added back in there.

searchresults.spec.js

    it("cleans up when closing by unclicking the categories", function () {
        var item = categories.querySelector("input");
        var close = sandpit.querySelector(".close");
        item.click();
        expect(item.checked).toBe(true);
        close.click();
        expect(item.checked).toBe(false);
    });

When adding back in the commented-out line, the test still fails. When I go to the spec list and select the one test that’s failing, I can see that the checkbox item is not being unchecked. Why is that?

After doing some debugging, I find that a mistake occurred in earlier code, where remove() was used when instead I only want to hide the result.

resultscontroller.js

    function closeClickHandler(evt) {
        ...
        // document.querySelector("#" + id).remove();
        $("#" + id).hide();
    }

After fixing that, the test still fails. Examining the searchresults code I find that the call to get the resultId isn’t right, and needs a second argument to say what the parent element is.

This is a good time to rename resultId so that it more correctly represents what is going on there. Renaming it to idFromParent seems better.

searchresults.js

            $(resultsArea).on("click", closeButton, function (evt) {
                // var value = resultsController.resultId(evt.target);
                var value = resultsController.idFromParent(evt.target, resultsSelector);
                console.log("button click");
                checkboxList.uncheckByValue(value);
            });

resultscontroller.js

    // function resultId(el, parentSelector) {
    function idFromParent(el, parentSelector) {
       ...
    }
    ...
    function closeClickHandler(evt) {
        ...
        var id = idFromParent(button, resultsSelector);
        ...
    }
    ...
    return {
        idFromParent: idFromParent,
        ...
    };

And the test passes, showing that we are testing for the right thing, and that the code all works.

Testing the form submit

We need to add a form to the sandbox that we are testing against.

searchresults.spec.js

        // sandpit = sandbox.init(`<p><input type="text"></p>
        sandpit = sandbox.init(`
            <form>
              <p><input type="text"></p>
              ...
            </form>
            <p>Results: <button id="clearAll" style="display: none">Clear all results</button></p>

The test that we’ll do is to add a character to the input field and trigger the submit event.

searchresults.spec.js

    it("can submit the selected categories", function () {
        var input = sandpit.querySelector("input");
        var item = categories.querySelector("input");
        input.value = "F";
        expect(item.checked).toBe(false);
        $(input).trigger("submit");
        expect(item.checked).toBe(true);
    });

I tried using just input.submit() but that caused the page to keep on reloading, so triggering the submit event instead allows the onsubmit event to fire.

Test the clearAll button

Checking the code coverage report for searchresults shows me that we’re just missing one last test, for when clicking the clearAll button.

First we need to remove the clearAll event so that we can have a failing test:

searchresults.js

            // $(clearAllButton).on("click", clearAllClickHandler);

searchresults.spec.js

    it("clears all of the results when the clearAll button is clicked", function () {
        var item = categories.querySelector("input");
        expect(isVisible(clearAllButton)).toBe(false);
        $(item).click();
        expect(isVisible(clearAllButton)).toBe(true);
    });

And now that we have a failing test, we can enable the click event and see that the test passes.

searchresults.js

            $(clearAllButton).on("click", clearAllClickHandler);

Important lessons learned today are:

  • be careful when changing the API to your code
  • simplify code where possible
  • writing code without having a test for it can result in problems that night not be caught
  • writing a test first before writing the code to make that test pass is a more reliable way to write code.

Next steps:

  • completely remove the need for the queue
  • run a fine-toothed comb over all of the tests
  • check the operation of the search-results code for anything that seems out of place
3 Likes

Whoops, the clearAll test was copied in the previous post from another test, but wasn’t updated to the code for actually testing if the clearAll button works.

Here’s that updated test now:

searchresults.spec.js

    it("clears all of the results when the clearAll button is clicked", function () {
        var item = categories.querySelector("input");
        var clearAll = sandpit.querySelector("#clearAll");
        $(item).click();
        expect(hasVisibleResults()).toBe(true);
        clearAll.click();
        expect(hasVisibleResults()).toBe(false);
    });
1 Like

The next thing on the list is to completely remove the queue. Here’s what we’ll do.

  • use a toggleResult function instead
  • clean up the searchresults code
  • remove the queue
  • remove other unneeded code

Use a toggleResult function instead

Currently the queue is mostly used in the searchresults updateResult function. If we can remove the need for the queue in there, we’ll be a long way towards removing the need for the queue.

Adding a toggleResult function to the resultsController code would help us to achieve that, so a failing test is first needed.

resultscontroller.spec.js

        it("can show a result", function () {
            var item = sandpit.querySelector("#Fruits");
            expect(hasVisibleResults()).toBe(false);
            resultsController.toggleResult(item);
            expect(hasVisibleResults()).toBe(true);
        });

Now that we have a failing test we can make it pass.

resultscontroller.js

    function toggleResult(result) {
        $(result).toggle();
    }

Well that was easy, but it’s not the only test. We also need it to hide a result that’s showing.

resultscontroller.spec.js

        it("can hide a result", function () {
            var item = sandpit.querySelector("#Fruits");
            $(item).show();
            expect(hasVisibleResults()).toBe(true);
            resultsController.toggleResult(item);
            expect(hasVisibleResults()).toBe(false);
        });

Oh, that still passes. What happens when we replace the searchresults updateResult function with a call to the resultscontroller toggleResult function?

searchresults.js

    function updateResult(checkbox, results) {
        // var value = checkbox.value;
        // if ($(checkbox).is(":checked")) {
        //     results.add(value);
        // } else {
        //     results.remove(value);
        // }
        // resultsController.updateResults(results.getAll());
        var result = $("#" + checkbox.value);
        resultsController.toggleResult(result);
    }

Only two tests fail that relate to the closeAll button. Can we easily fix that?

    function clearAllClickHandler() {
        $(searchField).val("").trigger("change").focus();
        $(":checked", categories).click();
        // $(closeButton, resultsArea).filter(":visible").click();
    }

Comment out one line, and all of the tests pass.

Welp, that was easy.

Cleaning up search results

What cleaning up needs to occur? That searchresult updateResult function isn’t really needed, and can have its contents moved to the doAfterClick function:

    // function updateResult(checkbox) {
    //     var result = $("#" + checkbox.value);
    //     resultsController.toggleResult(result);
    // }
    ...
                doAfterClick: function updateResults(checkbox) {
                    var result = $("#" + checkbox.value);
                    resultsController.toggleResult(result);
                    toggleClearAllBasedOnResults(resultsSelector);
                }

And, that toggleClearAllBasedOnResults function can be simplified too. Here’s what it currently looks like:

searchresults.js

    function toggleClearAllBasedOnResults(resultsSelector) {
        var hasResults = $(resultsSelector + ":visible").length > 0;
        $(clearAllButton).toggle(hasResults);
    }

Figuring out if there are any visible results, is the perfect thing to delegate to the resultscontroller code.

resultscontroller.spec.js

        it("can say if it has any visible results", function () {
            var item = sandpit.querySelector("#Fruits");
            expect(resultsController.hasVisible()).toBe(false);
            $(item).show();
            expect(resultsController.hasVisible()).toBe(true);
        });

which is a failing test, and can be made to pass by copying some of the code from toggleClearAllBasedOnResults over to the resultscontroller

resultscontroller.js

    function hasVisible() {
        return $(resultsSelector + ":visible").length > 0;
    }
    ...
    return {
        ...
        hasVisible: hasVisible,
        ...
    };

We can now remove the toggleClearAllBasedOnResults function, as we have a much easier way to achieve it.

searchresults.js

    // function toggleClearAllBasedOnResults(resultsSelector) {
    //     var hasResults = $(resultsSelector + ":visible").length > 0;
    //     $(clearAllButton).toggle(hasResults);
    // }
    ...
                doAfterClick: function updateResults(checkbox) {
                    var result = $("#" + checkbox.value);
                    resultsController.toggleResult(result);
                    $(clearAllButton).toggle(resultsController.hasVisible());
                }

Removing the queue

My code linter now tells me that the results queue is no longer used, so we can remove the last traces of the queue.

searchresults.js

    // var results = {};
    ...
        init: function init() {
            // results = Queue.init();

And like that, the queue is gone.

The resultscontroller tests that relate to the queue can now be removed:

resultscontroller.spec.js

        // it("shows an item when there was none before", function () {
        //     resultsController.updateResults([]);
        //     expect(hasVisibleResults()).toBe(false);
        //     resultsController.updateResults(["Fruits"]);
        //     expect(hasVisibleResults()).toBe(true);
        // });
        // it("removes an item when it was there before", function () {
        //     resultsController.updateResults(["Fruits"]);
        //     expect(hasVisibleResults()).toBe(true);
        //     resultsController.updateResults([]);
        //     expect(hasVisibleResults()).toBe(false);
        // });
        // it("shows nothing when there's nothing to show", function () {
        //     resultsController.updateResults([]);
        //     expect(hasVisibleResults()).toBe(false);
        //     resultsController.updateResults([]);
        //     expect(hasVisibleResults()).toBe(false);
        // });
        // it("shows all items when the update changes nothing", function () {
        //     resultsController.updateResults(["Fruits"]);
        //     expect(hasVisibleResults()).toBe(true);
        //     resultsController.updateResults(["Fruits"]);
        //     expect(hasVisibleResults()).toBe(true);
        // });

Remove other unneeded code

And we can now remove that hasVisibleResults function from the test, and use the resultscontroller function instead.

resultscontroller.spec.js

    // function hasVisibleResults() {
    //     var sandpitResults = $(opts.resultsSelector, sandpit);
    //     return $("*:visible", sandpitResults).length > 0;
    // }
    ...
        it("can show a result", function () {
            var item = sandpit.querySelector("#Fruits");
            // expect(hasVisibleResults()).toBe(false);
            expect(resultsController.hasVisible()).toBe(false);
            resultsController.toggleResult(item);
            // expect(hasVisibleResults()).toBe(true);
            expect(resultsController.hasVisible()).toBe(true);
        });
        ...

and so on for the other tests in there.

We can also remove the updateResults function from last remaining test that uses it:

resultscontroller.spec.js

    it("closes a result when the close button is clicked", function () {
        // resultsController.updateResults(["Fruits"]);
        var item = sandpit.querySelector("#Fruits");
        $(item).show();
        expect(resultsController.hasVisible()).toBe(true);
        $("#Fruits .close", sandpit).click();
        expect(resultsController.hasVisible()).toBe(false);
    });

That updateResults function is now not being used by anything, and so can also be removed.

resultscontroller.spec.js

    // function updateResults(items) {
    //     clearResults();
    //     showResults(items, "#results");
    // }
    ...
    return {
        ...
        // updateResults: updateResults,
        ...
    };

The clearResults and showResults functions aren’t being used either, and can be removed.

resultscontroller.js

    // function clearResults() {
    //     $(resultsSelector).hide();
    // }
    // function showResults(results, targetSelector) {
    //     if (results.length > 0) {
    //         results.forEach(function showResult(item) {
    //             $("#" + item)
    //                 .show()
    //                 .appendTo(targetSelector);
    //         });
    //     }
    // }
    ...
    return {
        ...
        // clearResults: clearResults,
        // showResults: showResults,
        ...
    };

And of course, the queue code can be deleted, and its tests can be removed too.

specRunner.html

    <!-- include source files here... -->
    <!--<script src="../src/queue.js"></script>-->
    ...
    <!-- include spec files here... -->
    ...
    <!--<script src="queue.spec.js"></script>-->

queue.js (delete)

// var Queue = (function () {
//     ...
// }());

queue.spec.js (delete)

// describe("Queue", function () {
//     ...
// });

index.html

<script src="lib/jquery.focusable.js"></script>
<!--<script src="src/queue.js"></script>-->
<script src="src/scroll.js"></script>

It’s a shame to see the queue go away after all the work to develop it, but as Stephen King once said:

Next steps:

  • grieve
  • run a fine-toothed comb over all of the tests
  • check the operation of the search-results code for anything that seems out of place
2 Likes

Today I’m going through the code with a fine-toothed comb which will involves:

  • An invisible Sandbox
  • Improving the tests and the code
  • Getting rid of globals

An invisible Sandbox

With the sandbox, I want to add a visible option to the sandbox so that it normally moves the sandbox out of sight, but can be made visible when debugging must occur.

To achieve this I want to make the html as an option, so that other options can be provided too.

sandbox.spec.js

        // var sandpit = sandbox.init(html);
        var sandpit = sandbox.init({
            html: html
        });

sandbox.js

    var initHtml = "";
    ...
    // function initSandpit(html) {
    function initSandpit(opts) {
        // initHtml = html;
        initHtml = (opts && opts.html) || "";
        ...
    }

That causes virtually all of the tests to fail, so we need to fix how the sandbox is being defined in the tests.

checkboxlist.spec.js

        // sandpit = sandbox.init(`<p><input type="text"></p>
        //         ...
        //         </div>`);
            sandpit = sandbox.init({
                html: `<p><input type="text"></p>
                    ...
                    </div>`
            });

After fixing how the sandbox is defined in all of the tests, I can now add that visible option.

sandbox.spec.js

    it("can have a visible sandbox", function () {
        var html = `<div id="anElement"></div>`;
        var sandpit = sandbox.init({
            html: html,
            visible: true
        });
        expect(sandpit.style.marginLeft).toBe("0px");
    });
    it("can have an invisible sandbox", function () {
        var html = `<div id="anElement"></div>`;
        var sandpit = sandbox.init({
            html: html,
            visible: false
        });
        expect(sandpit.style.marginLeft).toBe("-9999px");
    });
    it("defaults to not showing the sandbox", function () {
        var html = `<div id="anElement"></div>`;
        var sandpit = sandbox.init({
            html: html
        });
        expect(sandpit.style.marginLeft).toBe("-9999px");
    });

Which is achieved by adding an isVisible variable to the sandbox code, and defaults to not showing the sandbox.

sandbox.js

    var initHtml = "";
    var isVisible = true;
    ...
        // div.style.marginLeft = "-9999px";
        div.style.marginLeft = isVisible
            ? 0
            : "-9999px";
    ...
    function initSandpit(opts) {
        ...
        isVisible = (opts && opts.visible) || false;
        ...
    }

The other tests can now have tha visible option added to the sandbox, to provide a handy place to easily change the setting for that test:

checkboxlist.spec.js

        sandpit = sandbox.init({
            html: `<p><input type="text"></p>
            ...
            </div>`,
            visible: false
        });

So that later on if I need to debug a test, I can set visible to true and see what’s in the sandpit.
The tests run faster now too, as the screen isn’t being updated all of the time.

Improving the tests

I now want to scan through each of the sets of tests looking for improvements to make to them.

Improving sandbox tests

The beforeEach section of code that removes previous sandpits isn’t needed, because the sandbox already does that, so the beforeEach part can be removed.

sandbox.spec.js

    // beforeEach(function () {
    //   var box = document.body.querySelector("#sandbox");
    //   if (box) {
    //     document.body.removeChild(box);
    //   }
    });

The reset part of the sandbox isn’t used either now, so that can be removed.

sandbox.spec.js

    // it("Resets the sandbox", function () {
    //     var html = `<div id="anElement"></div>`;
    //     var sandpit = sandbox.init({
    //         html: html
    //     });
    //     sandpit.innerHTML = "";
    //     sandpit = sandbox.reset();
    //     expect(sandpit.innerHTML).toBe(html);
    // });

sandbox.js

    return {
        // reset: resetSandpit,
        ...
    };

Improve remove

sandbox.js

            // sandpit.parentNode.removeChild(sandpit);
            sandpit.remove();

Improve add

sandbox.js

    // function addSandbox() {
    function addSandbox(html) {
        ...
        // document.body.appendChild(div);
        div.innerHTML = html;
        return document.body.appendChild(div);
    }
    ...
        // addSandbox();
        // sandpit = document.body.querySelector("#sandbox");
        sandpit = addSandbox(initHtml);

And now the resetSandpit code is not needed at all.

sandbox.js

    // function resetSandpit() {
    //     return sandpit;
    // }
    ...
        // return resetSandpit();
        return sandpit();

It looks like the removeSandpit code isn’t used wither, so that can be removed too.

sandbox.js

    // function removeSandpit() {
    //     sandpit.parentNode.remove(sandpit);
    // }
    ...
    return {
        // remove: removeSandpit,
        ...
    };

So much code being removed, but this is what happens when you write code without writing tests first. You end up with ideas that just don’t end up being used.

Improving scroll tests

The scroll tests look like they rely far too much on jQuery, or at the very least have far too many $el variable names. It’s time to do something about that and simplify things.

scroll.spec.js

    // var item1;
    // var item2;
    // var item3;
    // var lastItem;
    var items;
    beforeEach(function () {
        ...
        // item1 = $("p:eq(0)", categories);
        // item2 = $("p:eq(1)", categories);
        // item3 = $("p:eq(2)", categories);
        // lastItem = $("p:last", categories);
        items = categories.children;
    });
    ...
        it("doesn't scroll if an element is already in view", function () {
            // var $el = $("p:first", $("#categories"));
            var item = items[0];
            categories.scrollTop = 0;
            // scroll.intoView($el);
            scroll.intoView($(item));
            expect(categories.scrollTop).toBe(0);
        });
    ...

Also since I’ve updated my jslint linter, parts of the tests have lines that are too long which helps to encourage me to use well-named variables to make the code easier to understand.

scroll.spec.js

    it("gets the distance between two separated elements", function () {
        // expect(scroll.outerDistanceBetween(item1, item3)).toBeCloseTo(-item1.get(0).offsetHeight, 0);
        var item1 = items[0];
        var item3 = items[2];
        var distance = scroll.outerDistanceBetween($(item1), $(item3));
        expect(distance).toBeCloseTo(-item1.offsetHeight, 0);
    });

That’s better now. The only place that jQuery is used now is when the function arguments are expected to be a jQuery object.

Improving scrollmanager tests

These tests all use variables called currentItem and expectedItem, which is so generic as to be nearly useless. It’s time to give all of these more expressive names.

For example:

scrollmanager.spec.js

        it("arrows up to the previous element", function () {
            // var currentItem = items[items.length - 1].querySelector("input");
            var lastItem = items[items.length - 1].querySelector("input");
            // var expectedItem = items[items.length - 2].querySelector("input");
            var prevItem = items[items.length - 2].querySelector("input");
            var item = issueItemKeyCommand(lastItem, "ArrowUp");
            // expect(item.id).toBe(expectedItem.id);
            expect(item.id).toBe(prevItem.id);
        });

The issueItemKeyCommand function could do with being less wordy as well.

scrollmanager.spec.js

    // function issueItemKeyCommand(target, key) {
    function issueKeyEvent(target, key) {
        ...
    }
    ...
        it("moves the page up focus instead of scrolling", function () {
            var firstItem = items[items.length - 1].querySelector("input");
            // var endItem = issueItemKeyCommand(firstItem, "End");
            var endItem = issueKeyEvent(firstItem, "End");
            // var pageUpItem = issueItemKeyCommand(endItem, "PageUp");
            var pageUpItem = issueKeyEvent(endItem, "PageUp");
            expect(pageUpItem.id).not.toBe(endItem.id);
        });

The code coverage tests show that the click code isn’t used anymore too, which can be removed.

scrollmanager.js

    // function clickHandler(evt, doAfterClick) {
    //     var el = evt.target;
    //     moveTo($(el));
    //     doAfterClick(el);
    // }
    // function clickWrapper(callback) {
    //     return function wrappedClickHandler(evt) {
    //         clickHandler(evt, callback);
    //     };
    // }
    ...
    return {
        ...
        // clickWrapper: clickWrapper,
        ...
    };

Improving the checkboxlist tests

The afterEach code is no longer needed now that the tests are all happening offscreen.

checkboxlist.spec.js

    // afterEach(function () {
    //     $("input", categories).each(function (ignore, input) {
    //         checkboxList.uncheckByValue($(input).val());
    //     });
    // });

And the keyDown function isn’t used anymore, now that other code has been moved off to a different place.

checkboxlist.spec.js

    // function keyDown(el, key) {
    //     var event = document.createEvent("Event");
    //     event.keyCode = key;
    //     event.initEvent("keydown");
    //     el.dispatchEvent(event);
    // }

The items selectors don’t need to use jQuery, and are simpler without it.

checkboxlist.spec.js

    var items;
    ...
    beforeEach(function () {
        ...
        items = categories.querySelectorAll("input");
        ...
    });
    ...
    it("selects the clicked category item", function () {
        // var firstItem = $("input:eq(1)", categories).get(0);
        var firstItem = items[0];
        // var secondItem = $("input:eq(1)", categories).get(0);
        var secondItem = items[1];
        firstItem.click();
        expect(document.activeElement).toBe(firstItem);
        secondItem.click();
        expect(document.activeElement).toBe(secondItem);
    });

Some significant improvements can be made now by removing jQuery from these tests.

checkboxlist.spec.js

    it("checks the first checkbox", function () {
        // expect($("input:eq(0)", categories).prop("checked")).toBe(false);
        // expect($("input:eq(1)", categories).prop("checked")).toBe(false);
        // expect($("input:eq(2)", categories).prop("checked")).toBe(false);
        expect(items[0].checked).toBe(false);
        expect(items[1].checked).toBe(false);
        expect(items[2].checked).toBe(false);
        checkboxList.checkFirst();
        // expect($("input:eq(0)", categories).prop("checked")).toBe(true);
        // expect($("input:eq(1)", categories).prop("checked")).toBe(false);
        // expect($("input:eq(2)", categories).prop("checked")).toBe(false);
        expect(items[0].checked).toBe(true);
        expect(items[1].checked).toBe(false);
        expect(items[2].checked).toBe(false);
    });

And just like that, we’ve easily removed jQuery from all of the checkboxlist tests.

Improving checkboxlist code

The doAfterClick part really could do with being renamed to the more standard name of afterClick

checkboxlist.js

    // function checkboxClickHandler(evt, doAfterClick) {
    function checkboxClickHandler(evt, afterClick) {
        var el = evt.target;
        scrollManager.moveTo($(el));
        // doAfterClick(el);
        afterClick(el);
    }
    ...
    function init(opts) {
        // var clickHandler = clickWrapper(opts.doAfterClick);
        var clickHandler = clickWrapper(opts.afterClick);
        ...
    }

checkboxlist.spec.js

        opts = {
            container: categories,
            // doAfterClick: handlers.doAfterClick
            afterClick: handlers.afterClick
        };

searchresults.js

            checkboxList.init({
                container: $(categories),
                afterClick: updateResults
            });

The afterClick code really should be optional, which also makes testing a bit easier, so we can check if opts.afterClick exists before assigning it.

checkboxlist.js

    function checkboxClickHandler(evt, afterClick) {
        ...
        // afterClick(el);
        if (afterClick) {
            afterClick(el);
        }
    }

And we can now remove these unneeded parts from the test

checkboxlist.spec.js

    // var handlers;
    ...
        // handlers = {
        //     afterClick: function () {
        //         return;
        //     },
        //     keyDownHandler: function () {
        //         return;
        //     }
        // };
        opts = {
            ...
            // afterClick: handlers.afterClick
        };

Improving resultsController

The idFromParent function name really needs to be improved. While it says what it does, it doesn’t express much intention. Calling it getName seems to be a better idea.

resultscontroller.js

    // function idFromParent(el, parentSelector) {
    function getName(el, parentSelector) {
        ...
    }
    ...
    function closeClickHandler(evt) {
        ...
        // var id = idFromParent(button, resultSelector);
        var id = getName(button, resultSelector);
        ...
    }
    ...
    return {
        // idFromParent: idFromParent,
        getName: getName,
        ...
    };

searchresults.js

    function uncheckCheckbox(evt) {
        // var value = resultsController.idFromParent(evt.target, resultSelector);
        var value = resultsController.getName(evt.target, resultSelector);
        ...
    }

resultscontroller.js

    function closeClickHandler(evt) {
        ...
        // var id = idFromParent(button, resultSelector);
        var id = getName(button, resultSelector);
        ...
    }

We should now be able to use the resultSelector that’s defined from the init code, instead of the parentSelector

resultscontroller.js

    // function getName(el, parentSelector) {
    function getName(el) {
        return $(el).parents(parentSelector).attr("id");
        return $(el).parents(resultSelector).attr("id");
    }
    function init(opts) {
        resultSelector = opts.resultSelector;
        ...
    }

which simplifies code that calls the function:

resultscontroller.js

    function closeClickHandler(evt) {
        ...
        // var id = getName(button, resultSelector);
        var id = getName(button);
        ...
    }

searchresults.js

    function uncheckCheckbox(evt) {
        // var value = resultsController.getName(evt.target, resultSelector);
        var value = resultsController.getName(evt.target);
        ...
    }

Improving searchResults code

The searchresults tests use jQuery to click items. We can remove that and just click the item itself now.

searchresults.spec.js

    it("clearAll button clears all of the results", function () {
        ...
        $(item).click();
        item.click();
        ...
    });

Getting rid of globals

I’m not happy that the code is using a lot of globals, such as scrollManager, checkboxList, resultsController, and searchResults. While it’s possible to move forward to using the ES6 export commands, there’s likely to be compatibilty issues with that so using RequireJS instead is a preferred solution.

To help migrate the code away from globals and towards a more organised system, we can put all of those globals into just the one global app object, and try to make sure that all references to parts of the app are passed in to the main function. That will help to prepare us for using a module system such as RequireJS.

So what I’ll do is the basic app conversion, carry on with going through the tests, and in a followup post convert everything over to be modules instead.

Adding scroll to the app

I don’t want the scroll script to fail if that is the only script that is loaded, so I need to use window.app to store the code, but assign it to an easily accessed app variable.

scroll.js

// var scroll = (function iife() {
window.app = window.app || {};
var app = window.app;
app.scroll = (function iife() {

We can then replace all occurrences of scroll throughout the rest of the code, with app.scroll instead.
The scroll.spec.js code has lots of references to scroll. We can easily deal with that by assigning scroll near the top of the code:

scroll.spec.js

describe("Scroll", function () {
    "use strict";
    var scroll = app.scroll;

This technique will help us with other code too. We’ll try to make sure that there’s only one entry point for each part of the app.

The scrollmanager code just needs to have scroll added to the function, so that it can be easily be made available to the code.

scrollmanager.js

var scrollManager = (function iife($) {
var scrollManager = (function iife($, scroll) {
    "use strict";
    ...
// }(jQuery));
}(jQuery, app.scroll));

And after testing to make sure that all of the tests still continue to pass, scrollManager is the next one to convert.

Adding scrollManager to the app

The same technique can be used with scrollManager. It’s so tempting to just race ahead and do all of the others, but that leaves the code in a non-working state where problems can’t be easily dealt with. it a step at a time lets you check that each step is without mistakes before moving on.

scrollmanager.js

// var scrollManager = (function iife($, scroll) {
window.app = window.app || {};
var app = window.app;
app.scrollManager = (function iife($, scroll) {

which is referenced in the scrollmanager tests and in checkboxlist

scrollmanager.spec.js

describe("Scroll manager", function () {
    "use strict";
    var scrollManager = app.scrollManager;

checkboxlist.js

// var checkboxList = (function iife($) {
var checkboxList = (function iife($, scrollManager) {
    ...
// }(jQuery));
// }(jQuery, app.scrollManager));

Adding checkboxList to the app

We can now add the checkboxList code to the app

checkboxlist.js

// var checkboxList = (function iife($, scrollManager) {
window.app = window.app || {};
var app = window.app;
app.checkboxList = (function iife($, scrollManager) {

which is used in the checkboxlist tests and the searchresults code

checkboxlist.spec.js

describe("Checkbox list", function () {
    "use strict";
    var checkboxList = app.checkboxList;

searchresults.js

// var searchResults = (function ($) {
var searchResults = (function ($, checkboxList) {
    ...
// }(jQuery));
}(jQuery, app.checkboxList));

Adding resultsController to the app

resultscontroller.js

// var resultsController = (function ($) {
window.app = window.app || {};
var app = window.app;
app.resultsController = (function ($) {

and the parts that use resultsController get updated:

resultscontroller.spec.js

describe("Results controller", function () {
    "use strict";
    var resultsController = app.resultsController;

searchresults.js

// app.searchResults = (function ($, checkboxList) {
app.searchResults = (function ($, checkboxList, resultsController) {
    ...
// }(jQuery, app.checkboxList));
}(jQuery, app.checkboxList, app.resultsController));

And that’s all the main parts added to a single app object for your code. Because of the earlier work that’s been done on the code, it’s been an easy conversion process.

That’s a huge number of updates today, but the code is smaller and easier to understand, and is ready to be transitioned over to using a common module system.

The updated code as it stands now can be downloaded from search-results-2018-06-13.zip

3 Likes

The code is now ready to be transitioned over to using a common module system.

The things being done today are:

  • install RequireJS
  • convert searchresults
  • migrate tests to use RequireJS
  • convert resultscontroller
  • convert checkboxlist
  • move filtercheckboxesboxesbytext
  • convert scrollmanager
  • convert scroll
  • convert filtercheckboxfromtext test
  • convert create-sandbox
  • convert sandbox

Using RequireJS

RequireJS just needs require.js from the download page, which can live in the lib folder.

SpecRunner.html

    <script data-main="../main.js" src="../lib/require.js"></script>
    <!-- <script>app.searchResults.init();</script> -->

We can remove the searchResults files from SpecRunner.html and move the init command into main.

SpecRunner.html

    <!-- <script src="../searchresults.js"></script> -->

main.js

require(["searchresults"], function (searchResults) {
    searchResults.init();
});

And we’re told that searchResults is undefined, so we just need to define searchResults as a module, where I can list checkboxList and resultsController requirements as dependencies of the code.

Converting searchresults

searchresults.js

// window.app = window.app || {};
// var app = window.app;
// app.searchResults = function ($, checkboxList, resultsController) {
define(
    ["jquery", "checkboxlist", "resultscontroller"],
    function ($, checkboxList, resultsController) {
        "use strict";
        var $ = jQuery;
        checkboxList = app.checkboxList;
        resultsController = app.resultsController;
        ...
// }(jQuery, app.checkboxList, app.resultsController));
    }
);

The test code now complains, which makes sense because it hasn’t been told how to get the search results.
We just need to wrap the test in a require statement, to make searchresults available for the test to use.

searchresults.spec.js

// describe("Search results", function () {
define(["searchresults"], function (searchResults) {
    "use strict";
    describe("Search results", function () {
        ...
            // app.searchResults.init(opts);
            searchResults.init(opts);
        ...
    });
});

and the tests all pass without trouble, except that search results don’t appear on the test page.

Migrate tests to use RequireJS

Instructions are easily found on integrating Jasmine with RequireJS so I’m renaming SpecRunner.html to SpecRunner-old.html, and moving the Jasmine library from js/lib/jasmine-2.6.4/ to js/tests/lib/ instead.

SpecRunner.html

<!DOCTYPE HTML>
<html>
<head>
    <meta charset="utf-8">
    <title>Jasmine Spec Runner v2.6.4</title>

    <link rel="shortcut icon" type="image/png" href="lib/jasmine_favicon.png">
    <link rel="stylesheet" href="lib/jasmine.css">
    <link rel="stylesheet" href="../../css/style.css">
</head>
<body>
    <script type="text/javascript" src="../lib/require.js" data-main="main"></script>
</body>
</html>

We can now configure require to load Jasmine:

tests/main.js

// Requirejs Configuration Options
require.config({
    // to set the default folder
    baseUrl: "../src",
    // paths: maps ids with paths (no extension)
    paths: {
        "jasmine": ["../tests/lib/jasmine"],
        "jasmine-html": ["../tests/lib/jasmine-html"],
        "jasmine-boot": ["../tests/lib/boot"]
    },
    // shim: makes external libraries compatible with requirejs (AMD)
    shim: {
        "jasmine-html": {
            deps: ["jasmine"]
        },
        "jasmine-boot": {
            deps: ["jasmine", "jasmine-html"]
        }
    }
});

And then, to bootstrap the Jasmine tests.

tests/main.js

require(["jasmine-boot"], function () {
    "use strict";
    require(["searchresults.spec"], function () {
        //trigger Jasmine
        window.onload();
    });
});

When the tests run, we’re told that jQuery is not defined, so we’ll copy across the jQuery libraries and the other code too. We can remove them when they have been properly defined.

SpecRunner.html

    <script type="text/javascript" src="../lib/require.js" data-main="main"></script>
    <script src="../lib/jquery-3.2.1.js"></script>
    <script src="../lib/escaperegexp.js"></script>
    <script src="../lib/jquery.focusable.js"></script>
    <script src="../lib/jquery.isscrollable.js"></script>
    <script src="../src/scroll.js"></script>
    <script src="../src/scrollmanager.js"></script>
    <script src="../src/filtercheckboxesbytext.js"></script>
    <script src="../src/checkboxlist.js"></script>
    <script src="../src/resultscontroller.js"></script>
    <script src="sandbox.js"></script>
    <script src="create-sandbox.js"></script>

We now have SpecRunner-old.html which runs without using require, and SpecRunner.html which uses the require modules.

Convert resultscontroller

It shouldn’t be all that difficult now to convert resultscontroller to being a required module.

SpecRunner-old.html

    <!-- <script src="../src/resultscontroller.js"></script> -->
    ...
    <!-- <script src="resultscontroller.spec.js"></script> -->

SpecRunner.html

    <!-- <script src="../src/resultscontroller.js"></script> -->

resultscontroller.js

// window.app = window.app || {};
// var app = window.app;
// app.searchResults = function ($, checkboxList, resultsController) {
define([], function () {
    "use strict";
    var $ = jQuery;
    ...
// }(jQuery));
});

I will leave jQuery as a global for now, until all of the other code has been converted over. It’s otherwise just too difficult to manage multiple versions of jQuery all at the same time.

searchresults now says that resultscontroller is undefined, so we can remove that app.resultscontroller line from it.

searchresults.js

        checkboxList = app.checkboxList;
        // resultsController = app.resultsController;

And with the tests working once more, we can bring back the resultscontroller tests by defining them as a module:

resultscontroller.spec.js

// describe("Results controller", function () {
define(["resultscontroller"], function (resultsController) {
    "use strict";
    describe("Results controller", function () {
        // var resultsController = app.resultsController;
        ...
    });
});

And we can now add resultscontroller to the tests

tests/main.js

    // require(["../tests/searchresults.spec"], function () {
    require(
        [
            "../tests/resultscontroller.spec",
            "../tests/searchresults.spec"
        ],
        function () {
            ...
        }
    );

Convert checkboxlist

The same process of remove, define, update, and spec, can now be done with checkboxlist.

SpecRunner-old.html

    <script src="../src/checkboxlist.js"></script>
    ...
    <script src="checkboxlist.spec.js"></script>

SpecRunner.html

    <script src="../src/checkboxlist.js"></script>

checkboxlist.js

// window.app = window.app || {};
// var app = window.app;
// app.checkboxList = function ($, scrollManager) {
define([], function () {
    "use strict";
    var $ = jQuery;
    var scrollManager = app.scrollManager;
    ...
// }(jQuery, app.scrollManager));
});

searchresults.js

        // checkboxList = app.checkboxList;

tests/main.js

        [
            "../tests/checkboxlist.spec",
            ...

checkboxlist.spec.js

// describe("Checkbox list", function () {
define(
    ["checkboxlist"],
    function (checkboxList) {
        "use strict";
        describe("Checkbox list", function () {
            // var checkboxList = app.checkboxList;
// });
        });
    }
);

Moving filtercheckboxesbytext

Because the movingcheckboxesbytext code adds a jquery method, it makes good sense to move the code into the lib folder with the other jquery code.

Move src/filtercheckboxesbytext → lib/jquery.filtercheckboxesbytext.js

SpecRunner-old.html

    <!-- <script src="../src/filtercheckboxesbytext.js"></script> -->
    <script src="../lib/jquery.filtercheckboxesbytext.js"></script>

Convert scrollmanager

The same process of remove, define, update, and spec, can now be done with scrollmanager.

SpecRunner-old.html

    <script src="../src/scrollmanager.js"></script>
    ...
    <script src="scrollmanager.spec.js"></script>

SpecRunner.html

    <script src="../src/scrollmanager.js"></script>

scrollmanager.js

// window.app = window.app || {};
// var app = window.app;
// app.scrollManager = (function iife($, scroll) {
define([], function () {
    "use strict";
    var $ = jQuery;
    var scroll = app.scroll;
    ...
// }(jQuery, app.scroll));
});

checkboxlist.js

// define([], function () {
define(["scrollmanager"], function (scrollManager) {
    "use strict";
    var $ = jQuery;
    // var scrollManager = app.scrollManager;

tests/main.js

        [
            "../tests/scrollmanager.spec",
            ...

scrollmanager.spec.js

// describe("Scroll manager", function () {
define(["scrollmanager"], function (scrollManager) {
    "use strict";
    describe("Scroll manager", function () {
        // var scrollManager = app.scrollManager;
        ...
    });
});

Convert scroll

The same process of remove, define, update, and spec, can now be done with scroll.

SpecRunner-old.html

    <script src="../src/scroll.js"></script>
    ...
    <script src="scroll.spec.js"></script>

SpecRunner.html

    <script src="../src/scroll.js"></script>

scroll.js

// window.app = window.app || {};
// var app = window.app;
// app.scroll = (function iife() {
define([], function () {
    "use strict";
    ...
// }(jQuery));
});

scrollmanager.js

// define([], function () {
// define(["scroll"], function (scroll) {
    "use strict";
    var $ = jQuery;
    // var scroll = app.scroll;

tests/main.js

        [
            "../tests/scroll.spec",
            ...

scroll.spec.js

// describe("Scroll", function () {
define(["scroll"], function (scroll) {
    "use strict";
    describe("Scroll", function () {
        // var scroll = app.scroll;
        ...
    });
});

Convert filtercheckboxfromtext test

This one is easier than the others, because it’s only the test that we’re converting.

SpecRunner-old.html

    <!-- <script src="filtercheckboxesbytext.spec.js"></script> -->

tests/main.js

        [
            "../tests/filtercheckboxesbytext.spec",
            ...

filtercheckboxesbytext.spec.js

// describe("Filter checkboxes by text", function () {
define([], function () {
    "use strict";
    describe("Filter checkboxes by text", function () {
        ...
    });
});

Convert create-sandbox

A similar process of remove, define, and spec, can now be done with create-sandbox.

SpecRunner-old.html

    <!-- <script src="create-sandbox.js"></script> -->
    <!-- <script src="create-sandbox.spec.js"></script> -->

SpecRunner.html

    <!-- <script src="create-sandbox.js"></script> -->

create-sandbox.js

// sandbox.create = (function iife() {
define([], function () {
    "use strict";
    // return function createSandbox() {
    sandbox.create = function () {
       ...
    };
}());

tests/main.js

// require(["jasmine-boot"], function () {
require(["jasmine-boot", "../tests/create-sandbox"], function () {

Convert sandbox

The last one we’ll be converting today is sandbox, for which the remove, define, update, and spec, can now be done.

SpecRunner-old.html

    <!-- <script src="sandbox.js"></script> -->
    <!-- <script src="sandbox.spec.js"></script> -->

SpecRunner.html

    <!-- <script src="sandbox.js"></script> -->

sandbox.js

// window.sandbox = (function iife() {
define([], function () {
    ...
// }());
});

We can now add sandbox as a requirement to any code that complains about not having it.

create-sandbox.js

// define([], function () {
define(["../tests/sandbox"], function (sandbox) {

filtercheckboxesbytext.spec.js

// define([], function () {
define(["../tests/sandbox"], function (sandbox) {
    ...
            // sandpit = window.sandbox.init({
            sandpit = sandbox.init({
    ...
});

checkboxlist.spec.js

// define(["checkboxList"], function (checkboxList) {
define(["../tests/sandbox", "checkboxList"], function (sandbox, checkboxList) {
    ...
            // sandpit = window.sandbox.init({
            sandpit = sandbox.init({
    ...
});

searchresults.spec.js

// define(["searchresults"], function (searchResults) {
define(["../tests/sandbox", "searchresults"], function (sandbox, searchResults) {
    ...
            // sandpit = window.sandbox.init({
            sandpit = sandbox.init({
    ...
});

scrollmanager.spec.js

// define(["scrollmanager"], function (scrollManager) {
define([""../tests/sandbox", scrollmanager"], function (sandbox, scrollManager) {
    ...
});

resultscontroller.spec.js

// define(["resultscontroller"], function (resultsController) {
define([""../tests/sandbox", "resultscontroller"], function (sandbox, resultscontroller) {
    ...
            // sandpit = window.sandbox.init({
            sandpit = sandbox.init({
    ...
});

scroll.spec.js

// define(["scroll"], function (scroll) {
define([""../tests/sandbox", "scroll"], function (sandbox, scroll) {
    ...
});

And the tests all pass. There are no more tests left in SpecRunner-old.html so we can now delete that file.

The only conversion left to do now is with jQuery, which I’ll leave for the next post.

3 Likes