Skip to content

Commit

Permalink
eliminate the need to run karma & webpack separately
Browse files Browse the repository at this point in the history 
TL;DR: running JS tests in canvas will be a lot faster & simpler

What you need to know as a developer writing JS in canvas:

Lets say you are working on the “dashboard_cards” feature, just run:
  `npm run jspec-watch spec/javascripts/jsx/dashboard_card`
While you write code and it will have a watcher that catches
any changes and re-runs just the dashboar_card specs if you save any
file that went into it. It should run & reload in less than a few
seconds. You can give it the path to a specific spec file or have it
run an entire directory.

Or, if you are working on something that might touch a lot of stuff, run:
  `npm run test-watch`
and while you are changing stuff, it will run *all* the QUnit specs on
any change. It should only take a couple seconds for webpack to process
the file change and to reload the specs in the browser.

Jenkins can now just run “npm test” for the webpack build. No need to
bundle install then run rake tasks just to run js tests.

This change also starts warning you when you have specs that take a
long time to run (e.g.: https://cl.ly/2i1O3O0J1504). It turns out we
have some *really* slow js specs (like selenium-level slow) so if you
notice a slow spec that you our your team wrote, please fix it.

Longer details:

To test our JS in webpack, we used to
1. run webpack with an env var set so it only does our ember stuff
2. run karma against ember
3. run webpack again against all the rest of the specs canvas
4 run karma again against all the specs in canvas
that took a long time. this change makes it so both the ember
specs and the specs in the rest of canvas run all in the same karma.
it also makes it so karma runs webpack itself. so you don’t need
to run `npm run webpack-test && karma start` to run tests, just
`npm run test` (which is just an alias to `karma start`). it also means
there is now just one watcher (karma) instead of one for both webpack
and karma.

Closes: CNVS-34977

Test plan:
* Jenkins builds should pass
* Try running different variations of the commands up there in the
  description. They should work and be fast-ish.

Change-Id: Ia97f9bfa3677763f218f5f02c9463344f180bc6c
Reviewed-on: https://gerrit.instructure.com/102169
Reviewed-by: Clay Diffrient <[email protected]>
Tested-by: Jenkins
Product-Review: Ryan Shaw <[email protected]>
QA-Review: Ryan Shaw <[email protected]>
  • Loading branch information
@ryankshaw
ryankshaw committed Feb 17, 2017
1 parent f3af6ad commit 1145e95
Show file tree
Hide file tree
Showing 15 changed files with 137 additions and 400 deletions.
3 changes: 2 additions & 1 deletion ...offeescripts/ember/screenreader_gradebook/tests/components/custom_column_cell.spec.coffee
View file
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
define [
'jquery'
'ember'
'../start_app'
'../shared_ajax_fixtures'
'compiled/gradebook/GradebookHelpers'
'jsx/gradebook/shared/constants'
], (Ember, startApp, fixtures, GradebookHelpers, GradebookConstants) ->
], ($, Ember, startApp, fixtures, GradebookHelpers, GradebookConstants) ->

{run} = Ember

Expand Down
3 changes: 2 additions & 1 deletion app/coffeescripts/ember/screenreader_gradebook/tests/components/grading_cell.spec.coffee
View file
Original file line number Diff line number Diff line change
@@ -1,9 +1,10 @@
define [
'jquery'
'ember'
'timezone'
'../start_app'
'../shared_ajax_fixtures'
], (Ember, tz, startApp, fixtures) ->
], ($, Ember, tz, startApp, fixtures) ->

{run} = Ember

Expand Down
2 changes: 1 addition & 1 deletion app/coffeescripts/handlebars_helpers.coffee
View file
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ define [
ret = ''

if context and context.length > 0
for index, ctx of context
for own index, ctx of context
ctx._index = index
ret += fn ctx
else
Expand Down
303 changes: 30 additions & 273 deletions doc/working_with_webpack.md
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
# Webpack!

Canvas is in the midst of transitioning from require_js to webpack. If
Canvas is almost done transitioning from require_js to webpack. If
you'd like, you can help. Canvas is currently equipped to run
with either frontend build to make the transition painless as possible.

Expand Down Expand Up @@ -34,8 +34,9 @@ for test and production can simply extend to add or remove elements as necessary
### Building javascript with webpack

If you look at package.json, you can see there are 2 scripts for building
your assets with webpack (webpack and webpack-production). Make sure you've
run "rake canvas:compile_assets" first so translations are in place,
your assets with webpack (webpack and webpack-production). If you want to include
the translations (eg when you run webpack-production), make sure you've
run "rake i18nliner:generate_js" first so translations are in place,
webpack depends on those already existing. You can use these scripts with "npm run":

`npm run webpack`
Expand All @@ -48,14 +49,13 @@ change things your updates will get compiled quickly.

This uglifies the resulting bundles, and takes longer. Don't use for development.

Webpack's output goes to "public/webpack-dist" for development js and
"public/webpack-dist-optimized" for minified production js.
Webpack's output goes to "public/dist/webpack-<dist or production>/".

### Using webpack javascript in canvas

The environment variable USE_WEBPACK is useful for trying out the assets
that webpack builds locally. If set to 'true', when you start your server
you'll load JS from your webpack-dist directory rather than public/js. You
you'll load JS from webpack rather than public/js. You
can do the same thing by touching the file "config/WEBPACK" (if present, it's
like having the USE_WEBPACK env var set).

Expand All @@ -67,23 +67,30 @@ to see webpack's js instead of the default requirejs loaded assets.

### Running js tests with webpack

We have built a seperate configuration for building one big test bundle
(webpack.test.config.js) which extends the default development config.
It builds the test bundle into spec/javascripts/webpack, and can be run
with the npm script "webpack-test".
Lets say you are working on the “dashboard_cards” feature, just run:
`npm run jspec-watch spec/javascripts/jsx/dashboard_card`
While you write code and it will have a watcher that catches
any changes and re-runs just the dashboard_card specs if you save any
file that went into it. It should run & reload in less than a few
seconds. You can give it the path to a specific spec file or have it
run an entire directory.

karma.conf.js has been adjusted to look for that bundle for testing instead
when the USE_WEBPACK environment variable is set.
Or, if you are working on something that might touch a lot of stuff, run:
`npm run test-watch`
and while you are changing stuff, it will run *all* the QUnit specs on
any change. It should only take a couple seconds for webpack to process
the file change and to reload the specs in the browser.

Which specs are included are right now configured manually in
"spec/javascripts/webpack_spec_index.js".
When karma starts, it automatically starts a webpack-dev-server and
webpacks whatever it needs to run the tests. see karma.conf.js for
more info on how it works.

To build the test bundle, you can run:
To run all the tests, you can run:

`npm run webpack-test`
`npm test`

Once you have the test bundle built, karma should work normally. I prefer
to run it headless in a container, so I run it with:
If you are using docker and want to run them all in a headless container you can
do so with with:

`docker-compose run --rm js-tests`

Expand Down Expand Up @@ -124,262 +131,18 @@ just run `rake canvas:compile_assets` for a fully up to date build.
## TODO List (update as needed):

*TASK LIST*
[X] setup context for quiz_submission_events.coffee so it doesn't have to
include the whole freaking bundles directory (fixed by removing errback)

[X] Make sure all bundles in app/coffeescripts/bundles (at least The
ones that actually get used) compile ok

[X] use loader to replace "worker" RequireJs
plugin

[X] loading partials from handlebars need to result
in partials registration

[X] loading bracketed partials in handlebars templates
must result in partials registration

[X] unbracketed nested partials need registration

[X] all handlebars helpers known to handlebars helper
should be counted as knownHelpers;

[X] all handlebars files need a dependency on
handlebars helpers

[X] make sure this works on node 0.12

[X] Bundles defined in plugins
like analytics must be
compiled into entries. Right
now we're specifying them all,
but we'll need to sniff for plugin
directories and find bundles
to add to the entry point list
(and to resolver paths).
No more symlinks.

[X] make sure we can still load
canvas_quizzes from core bundles.
Building client_apps will need
to be a pre-build task (js:build_client_apps).

[X] JSTs living in a plugin
like analytics or instructure_misc_plugin
must be available. Instead
of symlinks, this probably
means finding plugin directories
at runtime for webpack
and adding them as paths to the
config for resolving.

[X] split up webpack build to avoid memory limitation

[X] remove public/javascript/jsx
and start requiring jsx files
directly from their app/jsx
directory

[X] load JSX with a jsx webpack loader (babel).

[X] split vendor modules from app modules to reduce
build time and require it everywhere.

[X] Load jquery into an AMD friendly module (will need some kind of shim)

[X] Load i18n into a module (it doesn't export itself)

[X] Make sure backbone is required all the places it's revealed

[X] use common bundle to extract modules common
to many dependencies and require that first
in all environments.

[X] Change js_bundle helper to use webpack
bundles

[X] configure webpack to pick up chunks from bundle entry points in the right
folder (webpack-dist for now)

[X] Make sure this works
with the docker workflow

[X] Make sure _core_en_ translations are loaded before we start doing lookups (probably
in the loader)

[X] Sort out tinymce PluginManager loading (pluginManager is coming up undefined)
(and how to munge together tinymce plugin dependencies which expect a global library
and exposed submodules).

[X] fix HandlebarsHelper which can't find "registerHelper" as a function (probably loading wrong handlebars?)
(actually, was due to exports loader not being correctly applied)

[X] Timezone Loading is failing from the timezone plugin because the "load" function isn't defined
(needed to switch to using a return object, "load" was part of requirejs plugins)

[X] Solve module resolution error in ./app/coffeescripts/bundles/grade_question.coffee
(was a missing bundle, need to start sniffing for which bundles are actually in the
directory soon)

[X] test in development and
make sure we have source to
debug with

[X] Ask jon to show me how
i18nliner works in finding
translation keys and whether we
can perform it on source
files rather than
mid-compilation. If so,
than generating translations
would be a pre-build task.

[X] precompile handlebars templates through
i18nliner

[X] make sure backbone.js can load jquery off of window

[X] make i18nliner use a sync loader because async
is causing it to hang (false, all handlebars loads are completeing, checked
through log-grepping). Now figure out why we're hanging at the emit stage...
Ah, it's because the spawned processes are living forever. Add a "kill" to
each callback on completion so


[X] use canvas i18nliner instead of raw i18nliner

[X] make sure plugin i18n scopes run through i18nliner get
pickedup ok

[X] Too many processes being spawned by piping. Better do i18nliner transform
in process

[X] in the "Permissions" screen under accounts (and announcements & assignments under course),
View.coffee says "this.template is not a function"

[X] in "Discussions", discussions_topics_index.coffee says "Backbone is not defined"
(it was requiring only the "Router" attribute of backbone, but then referencing
Backbone globally)

[X] in the "Outcomes" nav item under accounts, learning_outcomes.coffee says "browserTemplate is not a function"

[X] on "Assignments" page for a course, "Uncaught TypeError: tz.parse is not a function"
by jquery.instructure_date_and_time (was using "require" instead of "define" in
timezone_plugin, require doesn't actually return a new module)

[X] when trying to create an announcment, "Uncaught TypeError: Cannot read property 'MS_TO_DEBOUNCE_SEARCH' of undefined" is thrown by DueDateTokenWrapper.jsx
("this" was actually undefined when they were using it outside of a function definition
in a class constructor)
[ ] convert the i18nliner task that extracts all the translations from source files to
be a babel loader so you can work against the same ast that babel already made and so
it can work against the es6 source and not the compiled output.

[X] ensure strings are still checked/exported w/o errors, i.e. `rake i18n:check` (it shells out to node stuffs)

[X] load up canvas w/ optimized js and RAILS_LOAD_ALL_LOCALES=1, switch your locale and ensure you see translations

[X] When loading a JST (probably a piped loader) will need to handle css registration

[X] Handle in-line partial registration when precompiling handlebars

[X] handle partial requirements from templates that depend on them

[X] When creating a new assignment, "Uncaught Error: The partial jst/assignments/_ submission_types_form could not be found"

[X] Talk with ryan about CDN
generation in the dist to make
sure that can be exported
on command.

[X] make sure webpack --watch
can deal with live changes in
development cycle

[X] upon requiring a module with extensions present (like plugins), make sure it loads the extensions (probably need to build a list of extensions up front from file system and check each require for it)

[X] When loading an ember JST
(hbs file), precompile through
i18nliner. (only can be tested in screenreader_gradebook)

[X] Make screenreader gradebook work

[X] test an ember app part of the site and make sure that still works

[X] Creating a quiz doesn't reference handlebars correctly

[X] assignments index, icons are missing, they don't know that scripts are loaded

[X] add ability to compress
outgoing builds.

[X] Make sure we can still use "with optimized js" environment variable for
local testing with prod-packaged code

[X] Replace or augment build steps in "rake canvas:compile_assets" pipeline with
webpack run based on USE_WEBPACK environment variable

[X] Build out the process that will probably work for getting code onto the CDN

[X] make "webpack-dist" rev-able and included in the CDN upload.

[X] make translations run before webpack if they aren't there

[X] Add basic docs.

[X] Make overriding webpack for requirejs (or vice versa) easy from a param
for testing

[X] make client-side
qunit tests work via webpack

[X] build a headless test runner, both for docker users and to avoid
that stupid browser window that tends to cache too much state.

[X] Discussion CSS seems slightly broken, apparently nested templates don't get parsed
correctly in i18nliner loader because of the differing root paths

[X]for screenreader_gradebook,
we could find files in
app/coffeescripts/ember/screenreader_gradebook and build
an entrypoint with a webpack plugin
that does all the work that
EmberBundle does now, but
it seems like an abstraction for one app. Since we aren't planning on
making any _more_ Ember
apps inside canvas (and Since
we probably want to kill this one), I say we make the generated "main.coffee" and
bundle files permenant and
committed to the git repo, and
yank EmberBundle out entirely.

[X] Extract an actual commons chunk

[X]what app code changes there are, move them out into seperate
small commits that can be tested individually

[X] in a seperate commit, remove all the shared ember components that aren't
used from "app/coffeescripts/ember/shared/components"

[X] get _all_ qunit tests running in the webpack bundle (just spiked on a few)

[X] on building for production, fails with ProximityLoader ("ERROR in 232.bundle.js from UglifyJs
Unexpected token: operator (!) [./frontend_build/jsHandlebarsHelpers.js!./frontend_build/pluginsJstLoader.js!./frontend_build/nonAmdLoader.js!./app/coffeescripts/util/ProximityLoader.coffee:111,6]")

[X] Migrate requires that are in views to application js (check plugins like mra)


[X] Delay returning tz in "timezone_plugin.js" until after promises have been run,
or change how the rest of the app interacts with timezone_plugin so that we can
return a promise, since async return is just not going to happen, and we need to have
those promises done before using it.

[ ] sort out scopes for .app.app.coffeescripts.ember.shared.templates.components.ic_submission-download-dialog so that we don't need an awful exception in 18n.js

[X] sniff test files automatically rather than configuring them manually in webpack_spec_index.js

[ ] could get a nice performance boost out of reimplementing BrandableCSS.all_fingerprints_for(bundle)
in node rather than shelling out to ruby for it

[ ] extract duplicated loader code from i18nLinerHandlebars and emberHandlebars

[ ] in a seperate commit extract i18nLinerHandlebars loader function and what
[ ] in a separate commit extract i18nLinerHandlebars loader function and what
it duplicates from prepare_hbs to a function both can use.

## After we're on webpack in production:
Expand All @@ -398,12 +161,6 @@ it duplicates from prepare_hbs to a function both can use.

[ ] delete our custom require-js plugins

[ ] migrate anything in bower to npm, dump bower

[ ] find other hard-coded vendor libs that can be ported to npm

[ ] rewrite qunit specs to actually have a qunit dependency and ditch that regex loader

[ ] kill the auto-css-in-handlebars imports (make them explicit) and remove that loader

[ ] plugins that need to be extended should have explicit extension points and let plugin register itself
Expand All @@ -412,4 +169,4 @@ it duplicates from prepare_hbs to a function both can use.

[ ] {low-priority} in chunks, take AMD requires off of our js files

[ ] {low-priority} re-write tests in mocha
[ ] Use Jest for future JS tests
Loading

0 comments on commit 1145e95

Please sign in to comment.