FEATURE: Introduce theme/component QUnit tests (PR #12517)

Proof of concept for theme testing.

GitHub

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? :thinking:

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 staff and 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 :+1: :heart:

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 theme_javascript_compiler.rb below):

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 getOwner() returns 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 start method.

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?

This pull request introduces 2 alerts when merging fda343ede00b8cc26dece859d13c75984c1ec407 into 42fb806f432573316b0805dc56e28c0e1bde4786 - view on LGTM.com

new alerts:

  • 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: