Proof of concept for theme testing.
Fine for now… but we may want to revisit how this works. Many theme developers work against production instances (e.g. theme-creator.discourse.org). Maybe this check should be more like current_user.admin?
General approach looks great here. Couple of random thoughts for the future:
A rake task to install a theme and run its tests (so we can run theme tests in CI)
Updates to theme-creator.discourse.org. Maybe a link in the editor UI so people can run tests
Thank you David! I only added this check because the
/qunit route does the same check, and yeah it’s definitely temporarily and I’ll revisit when theme testing is a little more fleshed out (didn’t want to do everything at once).
My plan is to add a hidden site setting that controls who can run JS tests. Allowed values for that setting would be
all. Theme creator would then be on
all and we would need to do some patching to allow the user to only run tests of themes they own.
Rake task to run a theme’s tests will definitely be added as well
Overall this is looking really nice, and I appreciate the initiative to get these working. I added a bunch of comments/questions.
I’m not sure of the value of the following few compiler tests. They seem quite brittle to me as if anything in the compiler changes they will fail. I think I’d rather look for a small substring rather than a block of compiled code.
For anything that’s registered we probably need an equivalent
reset() method that is called in the test environment to clean up after ourselves. Otherwise tests may leak.
__container__ is deprecated. Can we inject the service using
@service? If not do the
getOwner helpers not work?
I’m not sure why this is necessary. Each environment should already be setting the default owner. Did you get a failure without it?
Yeah I did get an error without this. Themes can access theme settings via a variable called
settings that we inject in themes code before we transpile them. The code that we inject looks like this (see the
theme_settings method in
const settings = require("discourse-common/lib/get-owner") .getOwner() .lookup("service:theme-settings") .getObjectForTheme(<theme id>);
This code is executed before the default owner is set by the
discourse-bootstrap initializer, and since no default owner has been set yet the
undefined. So, I needed to add this line to make the default owner available early enough so the theme settings lookup code doesn’t fail.
It’s possible that I may have missed something obvious here, so if you can think of a better way to solve this problem let me know and I’ll give it a try.
This is somewhat related to feature: theme tests (WIP) by OsamaSayegh · Pull Request #12517 · discourse/discourse · GitHub. This method is called by the
start method of the application, and at the time the
start method is called no owner has been set yet (I’m not sure if application instances have owners, but we could set one manually using
setOwner). So trying to access any injected services throws this assertion error:
Assertion Failed: Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container.
Similar story goes for the
getOwner helper if I try to use it here. Like feature: theme tests (WIP) by OsamaSayegh · Pull Request #12517 · discourse/discourse · GitHub, if I call
getOwner it returns
undefined unless I add the
setDefaultOwner call in the
It sounds to me like the problem is that the theme code is being called before the app has bootstrapped. Instead of adding
setDefaultOwner in start I would prefer us have the theme code called in the right order. Is that something you can handle?
- 2 for Unused variable, import, function or class
The title of this pull request changed from “feature: theme tests (WIP)” to "FEATURE: Introduce theme/component QUnit tests
@eviltrout / @davidtaylorhq I’ve made a fair amount of changes since your last review, so could you please give this another review and let me know if there is anything you’d like me to change? Thanks!
This is looking quite nice now. I think we should merge when you are around to support in case of any issues, then follow up with fixes as needed.
This pull request has been mentioned on Discourse Meta. There might be relevant details there: