Vexflow: Clean up tests

Created on 24 Jul 2016 · 8Comments · Source: 0xfe/vexflow

What I see as the current issues with the tests:

Not using ES6
Inconsistent style -- not linted
Tons of duplication
- No test helpers around graphical objects
No clear pattern for writing a test
- objects constructed, formatted, draw in different orders for different tests -- often in ways that are not intended to work, but happen to anyway
Lack of clarity
- Too much test code is focused on boilerplate

Update (Aug 28th, 2016)

The addition of the Factory and EasyScore is a massive win for the last 3 points. I've begun refactoring a lot of the tests to use these classes. See https://github.com/0xfe/vexflow/pull/433, https://github.com/0xfe/vexflow/pull/431, https://github.com/0xfe/vexflow/pull/423, https://github.com/0xfe/vexflow/pull/420, https://github.com/0xfe/vexflow/pull/417

It's worth noting that VexFlow is soon going to a add a new requirement: a Stave must be set on all appropriate Elements before formatting (see https://github.com/0xfe/vexflow/pull/412 for the relevant discussion). However, the following tests still do not satisfy this requirement (see list at bottom of this post)

Basic voice creation can probably use EasyScore (wiki) while more complex test setup might require extending and using the Factory.

Example of refactoring using EasyScore - #433
Example of refactoring using only the Factory - #417

However we still have the problem that we can't use ES6 -- I think we should transpile the tests when running them in a headless browser that requires ES5 (ie: PhatomJS).

List of remaining tests to fix -- updated 24/12/2016

NONE. Wooooooo

maintenance

Source

Silverwolf90

Most helpful comment

I've nearly completed a first pass over the tests (and got some bug fixes out of it too)! I don't think I hit every single file, only files with tests that failed the stave requirement mentioned in the original post. But this means that when all the outstanding test PRs are merged in, we can finally fix VexFlow's vertical formatting!!! (eg: https://github.com/0xfe/vexflow/pull/412 https://github.com/0xfe/vexflow/issues/411)

👍 👍 👍 👍 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 🎉 👍 👍 👍 👍

A lot of tests now use a mix of Factory and EasyScore (for the more trivial setup). But some I just reformatted/linted/slightly-restructured.

All the work is currently open in PRs or already merged in (see: #508 #507 #506 #505 #504 #503 #502 #501 #499 #498 #497 #496 #495 #492 #491 #490 #488 #487 #486 #459 #458 #456 #433 #431 #423 #422 #420 #417)

It's not over... this was only a first pass...

We really need a solid audit of the current suite. Tests should exist for precise reasons. A lot of tests are named like "Rests - multi voice," but this is completely inadequate. What about multi voice rests are we testing specifically? Those cases need to be defined and probably colocated with the test itself.

A lot of tests have a ton of unnecessary setup. If a test is specifically constructed around a case, then that case should be isolated without any superfluous elements. Often, it's not at all clear why specific setup was chosen and what code-paths are meant to be hit.

The sheer amount of copy-paste tests was brutal. If duplication is necessary it should be extracted out (eg: https://github.com/0xfe/vexflow/pull/486). In the past I've been extremely guilty of this and it cost a lot of time during the refactor :P I also didn't fix the duplication in every file, some were easy to do as part of fixing the file, others deviated enough from test to test that it will need to happen at another time.

As mentioned above, having code coverage would be huge so that we could be code-path aware. We have a lot of tests that have questionable value because they only hit well-traveled code-paths.

Silverwolf90 on 24 Dec 2016

🎉1 👍1

All 8 comments

Could I also add that there's no coverage reporting? The tests are very focused on visual rendering and diffing (understandably, the highest priority), but there's a bunch of logic to be tested as well.

aaronmars on 24 Jul 2016

👍1

Original post has been updated with the current state of things.

Silverwolf90 on 28 Aug 2016

Cool!

I would say though that, "No clear pattern for writing a test: objects constructed, formatted, draw in different orders for different tests," can also be a positive thing to try to avoid factoring out too much -- the more different ways objects are constructed, etc. the more parts of the system are being flexed.

mscuthbert on 28 Aug 2016

@mscuthbert Agreed! Code coverage would give us a concrete number around this -- that would be awesome.

Edited that bullet point to clarify my intention.