Add JS tests covering more modules #142
Comments
fedarko
added a commit
to fedarko/empress
that referenced
this issue
Apr 23, 2020
ElDeveloper
pushed a commit
that referenced
this issue
May 12, 2020
…fix a tiny bug (#159) * TST: Add test-colorer.js skeleton #142 #137 * TST: Test that QIIME colorscheme matches Emperor :100: * BUG/ENH: Fix discrete colormaps (close #137) This replaces Colorer.__colorer with Colorer.__colorArray, which is what it sounds like on the tin (an array of hex color codes) :) The downside is that this treats quant. color schemes the same way as discrete color palettes: so these palettes are just looped, rather than being scaled to fit the entire range of items. I'm of the opinion that applying quant. color schemes to discrete data is a bit useless, so I think the next step is adding some sort of button / checkbox / etc. that lets the user select if they want color setting to be done with the looping method (added here) or the scaling method (as was done previously). Will need to think about what provides the best UX (I think these defaults will be better than before, at least). * TST: test discrete colormap construction for #137 * BUG/TST: Fix color precision bug; add tst for RGB 255 instead of 256 for scaling. See new comment in colorer.js for explanation. realized that prettier wasn't being run on test js code, so just pulled that trigger. will commit changes for remaining test-*.js files shortly. * TST: ... actually declare module for colorer tsts WHOOPS didn't realize that keeping that line was important * TST: test getColorHex() annnd I think we're good here! * DOC: finally, redo qzv * MNT: Simplify getColorRGB() significantly Turns out chroma(some color).gl() is a function! dang. * TST: use chroma(color).hex() as expected rather than assuming that chroma(color)'s default repr will be as hex (I think that's a safe assumption, but best not to bake too many assumptions into the tests...) * MNT: Add util.js with naturalSort() Per @ElDeveloper's suggestion in #159 * BUG: utils -> util in module name references oops, that broke the js Ok, TODOs from here are: -Use util.naturalSort() to sort categories, both for coloring AND for placement in the legend (or ideally, just sort once and reuse the results) -If a sequential / diverging colormap is selected, interpolate values analogously to what Emperor does. This will just be "ordinal" for now, so e.g. [1, 2, 3, 4] will result in the same colors assigned as [1, 100, 1000, 10000000] or any other 4-category field. * TST: Port over util.naturalSort tests #159 Includes creating a new test-util.js module. (I imagine we'll probably add more util functions and their tests to util.js + test-util.js in the future.) * BUG: use naturalSort() for all sample categories This could be made more efficient (i.e. only doing the sorting op. once and saving the result rather than doing it in two diff. places) but just wanna get this working for now. Weirdly enough the color stuff seems to be malfunctioning now? I don't think this commit is the cause but I think something on this branch got messed up... I'll check * BUG: Fix funky WebGL coloring bug See test comments for explanation. TLDR: WebGL (or maybe some intermediate utility) misinterprets the opacity component in the color array, which breaks things. ACTUALLY: thinking about it now, I think this bug might actually be due to Empress' code, which mixes up colors and coordinates into a single array when drawing stuff. I bet that that's it -- the "1" is probably interpreted as a coordinate, which is why things look so weird. * DOC: update comments re: cause of prev bug * MNT: Add splitNumericValues() & port Emperor tests * MNT: Move keepUniqueKeys() to util module I assume this function could be useful for other contexts, and porting it to this module should make testing it a bit easier (which will be useful when refactoring it; #147). I haven't actually added tests for this function yet, mostly because I'm having a hard time following it / understanding what it's doing. * ENH: Scale numeric vals for SEQ/DIV colormaps Addresses @ElDeveloper's comments in #159. The code for this is a bit clunky (a lot of logic should be moved out of the Colorer constructor; the alerts happen twice when they should only happen once; tests are broken), but from here on should be ok * MNT: Simplify Colorer obj a lot Rather than creating multiple Colorers for the RGB and Hex stuff, this just creates one Colorer and reuses it for both color formats. This lets us completely do away with Empress._assignColor()! And some other stuff, too. * MNT: Split up Colorer constructor to sep funcs also updated QZV, verified that it seems to work (still need to update colorer tests tho) * TST: Update discrete color palette test * MNT/TST: Test color scaling; bump chroma to v2.1 Turns out that the Chroma.js version we were using here was ~4 years out of date -- I noticed small discrepancies (like, on the order of ~0x01 precision for R/G/B channels of the hex colors) between our Chroma.js and the Chroma.js docs when writing this test. It looks like Chroma.js version 1.3.3 improved precision, which is probably the (main?) cause of the observed discrepancies. I confirmed that updating the version fixed the test errors. This implies that we should bump the Chroma version for Emperor / other libraries using old versions of Chroma also. * TST: add more color scaling tests * TST: Finally, test the new Colorer.getMap funcs * DOC/BUG: Document getMap funcs; minor class thing See getMapHex()'s interior for some details on the change made in this commit * TST/MNT: Defer alert on scaling fail to post tests This is really just kicking the can down the road, but we can do that ad infinitum as needed I guess. (... or we can just bite the bullet and inevitably spend like 10 hours messing with Puppeteer) * DOC: update screenshot now that #137 fixed * BUG: fix inconsistent legend order Needed to slap on a call to naturalSort() in the legend code. It would be best to instead just sort once and pass the sorted list through the chain of code... that's a TODO. (Also testing would help make me more confident about this :) * MNT: Replace "ccm" scaling with "ccwm" scaling So, things are now scaled ordinally. See comments of new function in colorer.js for details. * TST: Update colorer tests re: ordinal scaling chgs * BUG: Update animation to work with new Color API This seems to function as expected, but it would be good to check that this didn't break things * BUG/TST: Fix and test single-value color case Forgot about this. should be good now. * BUG: Put Infinities with words in naturalSort * DOC: update demo QZV to use latest JS from this PR
This was referenced Jun 26, 2020
Merged
Merged
Merged
fedarko
added a commit
to fedarko/empress
that referenced
this issue
Jul 18, 2020
If thickening *was* done previously, but wasn't currently being done, then Empress._currentLineWidth wasn't being updated to 0. This meant that, when redrawing the tree after updating the layout, an old line width was being used. Now things are good! yay. (This is the sort of case we should probs add a test for. tagging biocore#142 accordingly.)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Listing JS files currently in
empress/support_files/js/and whether or not atest-[filename].jsfile already exists for the file:animation-panel-handler.jsanimator.jsbiom-table.jsbp-tree.jsbyte-array.jscamera.jscanvas-events.jscolorer.jsdrawer.jsempress.jslegend.jsselect-node-menu.jsside-panel-handler.jsutil.jsvector-ops.jsI imagine we'd want to cover all or at least most of these files eventually. Adding tests for more complex modules that rely on page state (e.g.
empress.js) may be tricky, but should be doable. (And on the other side of things, some files, e.g.vector-ops.js, hopefully shouldn't require much effort to test -- since they just expose utility functions that can be unit-tested in isolation.)One more note, somewhat related: adding JS code coverage via CodeCov would also help make identifying untested portions of the code easier (I rely on digging through coverage a lot, esp when testing JS code). That being said, this might require us to use a different test runner than
qunit-puppeteer, since I don't think that it currently outputs JS test coverage information.The text was updated successfully, but these errors were encountered: