-
Notifications
You must be signed in to change notification settings - Fork 31
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
Conversation
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
The following artifacts were built for this PR: empire-biplot.qzv, empire.qzv, empress-tree.qzv |
There was a problem hiding this 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.
tests/test-barplot-panel-handler.js
Outdated
notOk(empress2._barplotPanel.availContent.classList.contains("hidden")); | ||
ok(empress2._barplotPanel.unavailContent.classList.contains("hidden")); | ||
}); | ||
// test("Layout availability toggling post-initialization", function () { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
Thanks @fedarko! |
This closes #316. If no feature metadata is used when generating a QZV, barplot layers now just show the sample metadata barplot controls:
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 moduletest-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 thesetup
block oftest-empress.js
to a new module (utilities-for-testing.js
, named that way in order to avoid confusion withutil.js
andtest-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 thegl-matrix
code: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.)