Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hide feature metadata barplot controls if no feature metadata is available; add first barplot panel tests; abstract Empress initialization code for JS testing #320

Merged
merged 6 commits into from
Aug 13, 2020

Conversation

fedarko
Copy link
Collaborator

@fedarko fedarko commented Aug 11, 2020

This closes #316. If no feature metadata is used when generating a QZV, barplot layers now just show the sample metadata barplot controls:

image

To make sure this isn't a problem again, this PR also adds the first tests for the barplot-panel-handler code* (although the tests interact with the barplot layer code as well, so I guess we could also just call the module test-barplots or something...). Part of this required re-creating an Empress object, so to make life easier for us in the future I abstracted that code from the setup block of test-empress.js to a new module (utilities-for-testing.js, named that way in order to avoid confusion with util.js and test-util.js ._.).

I don't think it's required to merge this PR in, but there is an issue with the barplot panel tests I haven't been able to figure out yet -- basically, when calling Empress.updateLayout() during tests, I get a weird error from the gl-matrix code:

TypeError: Cannot set property '0' of undefined
    at Object.u (file:///home/marcus/Dropbox/Work/KnightLab/Empress/fedarko/empress/empress/support_files/vendor/gl-matrix.min.js:28:30362)
    at Drawer.centerCameraOn (file:///home/marcus/Dropbox/Work/KnightLab/Empress/fedarko/empress/empress/support_files/js/drawer.js:466:17)
    at Empress.centerLayoutAvgPoint (file:///home/marcus/Dropbox/Work/KnightLab/Empress/fedarko/empress/empress/support_files/js/empress.js:1896:22)
    at Empress.updateLayout (file:///home/marcus/Dropbox/Work/KnightLab/Empress/fedarko/empress/empress/support_files/js/empress.js:1783:22)
    at Object.<anonymous> (file:///home/marcus/Dropbox/Work/KnightLab/Empress/fedarko/empress/tests/test-barplot-panel-handler.js:80:17)

I can trace the error back to this line of code, but I'm not familiar enough with this to say what the problem is. Since centering the camera works perfectly fine when running Empress :), I assume that this is a problem with mocking the canvas / drawer / camera / etc. during testing that is breaking WebGL somehow. The test this error impacts isn't very critical, so I've just commented it out for now. If we can get it working, though, that'd be nice.

* (There were already tests for many of the functions used by the barplot code, but no direct tests yet for things like the barplot HTML. This PR should make it easier to add more of those.)

Also shuffled around the order of test JS modules in the index.html
file -- looked like things got moved around, but i don't think it
made a difference
some problems with calling updateLayout() so testing that will
have to wait for now, but this is nice
i mean it's still broken but now if we fix the gl stuff the test
should work lol
@emperor-helper
Copy link

The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv

Copy link
Collaborator

@kwcantrell kwcantrell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @fedarko! I just have one comment.

notOk(empress2._barplotPanel.availContent.classList.contains("hidden"));
ok(empress2._barplotPanel.unavailContent.classList.contains("hidden"));
});
// test("Layout availability toggling post-initialization", function () {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The error is caused by line 453 in drawer because treeSpaceCenterX and treeSpaceCenterY are defined by taking the offsetWidth/offsetHeight of treeContainer which is an html component that is not defined in the test. So, when glMatrix tries to multiply on line 465, it cant because centerMat has undefined entries. You should be able to fix this by adding an div with the id 'tree-container' to utilities-for-testing

Copy link
Collaborator Author

@fedarko fedarko Aug 11, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe this is the problem -- a div with the ID tree-container is already defined in tests/index.html, and it has offset width / height properties (document.getElementById("tree-container").offsetWidth; produces a number when run in the console of tests/index.html).

Did some digging and I think I've managed to solve the problem (this test works now, at least). the problem was that empress.initialize() (which calls drawer.initialize()) wasn't being called anywhere (in the non-test Empress code it's only called in the empress-template.html file). Took care of this by calling the function within the test, but then more errors came up -- but they boiled down to other HTML elements on the page not existing, like you said. I added for the missing elements and now this test works??? (edit: I realize now that the test-empress.js file already initialized just the drawer, so these changes may have been overkill... ah well things work at least :)

The testing HTML is getting kind of bloated so we may want to try to auto-populate it in the future... but for now the tests work at least :P

(As a nice side note, I think this should make testing select-node-menu / canvasEvents / drawer more feasible down the line)

@kwcantrell
Copy link
Collaborator

Thanks @fedarko!

@kwcantrell kwcantrell merged commit 6a7b9c4 into biocore:master Aug 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Hide "feature metadata" barplot controls when no feature metadata was used to generate the visualization
3 participants