mirror of
https://github.com/zulip/zulip.git
synced 2025-11-02 04:53:36 +00:00
Remove rST files to replace with Markdown.
Currently (https://github.com/rtfd/recommonmark/issues/3) the recommonmark bridge (which allows Sphinx to read Markdown) does not support tables, so the directory structure doc is now a bulleted list instead of a set of tables.
This commit is contained in:
committed by
Tim Abbott
parent
e0bc57ddb1
commit
337155f280
482
docs/code-style.md
Normal file
482
docs/code-style.md
Normal file
@@ -0,0 +1,482 @@
|
||||
Code style and conventions
|
||||
==========================
|
||||
|
||||
Be consistent!
|
||||
--------------
|
||||
|
||||
Look at the surrounding code, or a similar part of the project, and try
|
||||
to do the same thing. If you think the other code has actively bad
|
||||
style, fix it (in a separate commit).
|
||||
|
||||
When in doubt, send an email to <zulip-devel@googlegroups.com> with your
|
||||
question.
|
||||
|
||||
Lint tools
|
||||
----------
|
||||
|
||||
You can run them all at once with
|
||||
|
||||
./tools/lint-all
|
||||
|
||||
You can set this up as a local Git commit hook with
|
||||
|
||||
``tools/setup-git-repo``
|
||||
|
||||
The Vagrant setup process runs this for you.
|
||||
|
||||
`lint-all` runs many lint checks in parallel, including
|
||||
|
||||
- Javascript ([JSLint](http://www.jslint.com/))
|
||||
|
||||
> `tools/jslint/check-all.js` contains a pretty fine-grained set of
|
||||
> JSLint options, rule exceptions, and allowed global variables. If
|
||||
> you add a new global, you'll need to add it to the list.
|
||||
|
||||
- Python ([Pyflakes](http://pypi.python.org/pypi/pyflakes))
|
||||
- templates
|
||||
- Puppet configuration
|
||||
- custom checks (e.g. trailing whitespace and spaces-not-tabs)
|
||||
|
||||
Secrets
|
||||
-------
|
||||
|
||||
Please don't put any passwords, secret access keys, etc. inline in the
|
||||
code. Instead, use the `get_secret` function in `zproject/settings.py`
|
||||
to read secrets from `/etc/zulip/secrets.conf`.
|
||||
|
||||
Dangerous constructs
|
||||
--------------------
|
||||
|
||||
### Misuse of database queries
|
||||
|
||||
Look out for Django code like this:
|
||||
|
||||
[Foo.objects.get(id=bar.x.id)
|
||||
for bar in Bar.objects.filter(...)
|
||||
if bar.baz < 7]
|
||||
|
||||
This will make one database query for each `Bar`, which is slow in
|
||||
production (but not in local testing!). Instead of a list comprehension,
|
||||
write a single query using Django's [QuerySet
|
||||
API](https://docs.djangoproject.com/en/dev/ref/models/querysets/).
|
||||
|
||||
If you can't rewrite it as a single query, that's a sign that something
|
||||
is wrong with the database schema. So don't defer this optimization when
|
||||
performing schema changes, or else you may later find that it's
|
||||
impossible.
|
||||
|
||||
### UserProfile.objects.get() / Client.objects.get / etc.
|
||||
|
||||
In our Django code, never do direct `UserProfile.objects.get(email=foo)`
|
||||
database queries. Instead always use `get_user_profile_by_{email,id}`.
|
||||
There are 3 reasons for this:
|
||||
|
||||
1. It's guaranteed to correctly do a case-inexact lookup
|
||||
2. It fetches the user object from remote cache, which is faster
|
||||
3. It always fetches a UserProfile object which has been queried using
|
||||
.selected\_related(), and thus will perform well when one later
|
||||
accesses related models like the Realm.
|
||||
|
||||
Similarly we have `get_client` and `get_stream` functions to fetch those
|
||||
commonly accessed objects via remote cache.
|
||||
|
||||
### Using Django model objects as keys in sets/dicts
|
||||
|
||||
Don't use Django model objects as keys in sets/dictionaries -- you will
|
||||
get unexpected behavior when dealing with objects obtained from
|
||||
different database queries:
|
||||
|
||||
For example,
|
||||
`UserProfile.objects.only("id").get(id=17) in set([UserProfile.objects.get(id=17)])`
|
||||
is False
|
||||
|
||||
You should work with the IDs instead.
|
||||
|
||||
### user\_profile.save()
|
||||
|
||||
You should always pass the update\_fields keyword argument to .save()
|
||||
when modifying an existing Django model object. By default, .save() will
|
||||
overwrite every value in the column, which results in lots of race
|
||||
conditions where unrelated changes made by one thread can be
|
||||
accidentally overwritten by another thread that fetched its UserProfile
|
||||
object before the first thread wrote out its change.
|
||||
|
||||
### Using raw saves to update important model objects
|
||||
|
||||
In most cases, we already have a function in zephyr/lib/actions.py with
|
||||
a name like do\_activate\_user that will correctly handle lookups,
|
||||
caching, and notifying running browsers via the event system about your
|
||||
change. So please check whether such a function exists before writing
|
||||
new code to modify a model object, since your new code has a good chance
|
||||
of getting at least one of these things wrong.
|
||||
|
||||
### `x.attr('zid')` vs. `rows.id(x)`
|
||||
|
||||
Our message row DOM elements have a custom attribute `zid` which
|
||||
contains the numerical message ID. **Don't access this directly as**
|
||||
`x.attr('zid')` ! The result will be a string and comparisons (e.g. with
|
||||
`<=`) will give the wrong result, occasionally, just enough to make a
|
||||
bug that's impossible to track down.
|
||||
|
||||
You should instead use the `id` function from the `rows` module, as in
|
||||
`rows.id(x)`. This returns a number. Even in cases where you do want a
|
||||
string, use the `id` function, as it will simplify future code changes.
|
||||
In most contexts in JavaScript where a string is needed, you can pass a
|
||||
number without any explicit conversion.
|
||||
|
||||
### Javascript var
|
||||
|
||||
Always declare Javascript variables using `var`:
|
||||
|
||||
var x = ...;
|
||||
|
||||
In a function, `var` is necessary or else `x` will be a global variable.
|
||||
For variables declared at global scope, this has no effect, but we do it
|
||||
for consistency.
|
||||
|
||||
Javascript has function scope only, not block scope. This means that a
|
||||
`var` declaration inside a `for` or `if` acts the same as a `var`
|
||||
declaration at the beginning of the surrounding `function`. To avoid
|
||||
confusion, declare all variables at the top of a function.
|
||||
|
||||
### Javascript `for (i in myArray)`
|
||||
|
||||
Don't use it:
|
||||
[[1]](http://stackoverflow.com/questions/500504/javascript-for-in-with-arrays),
|
||||
[[2]](http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#for-in_loop),
|
||||
[[3]](http://www.jslint.com/lint.html#forin)
|
||||
|
||||
### jQuery global state
|
||||
|
||||
Don't mess with jQuery global state once the app has loaded. Code like
|
||||
this is very dangerous:
|
||||
|
||||
$.ajaxSetup({ async: false });
|
||||
$.get(...);
|
||||
$.ajaxSetup({ async: true });
|
||||
|
||||
jQuery and the browser are free to run other code while the request is
|
||||
pending, which could perform other Ajax requests with the altered
|
||||
settings.
|
||||
|
||||
Instead, switch to the more general `$.ajax`\_ function, which can take
|
||||
options like `async`.
|
||||
|
||||
### State and logs files
|
||||
|
||||
Do not write state and logs files inside the current working directory
|
||||
in the production environment. This will not how you expect, because the
|
||||
current working directory for the app changes every time we do a deploy.
|
||||
Instead, hardcode a path in settings.py -- see SERVER\_LOG\_PATH in
|
||||
settings.py for an example.
|
||||
|
||||
JS array/object manipulation
|
||||
----------------------------
|
||||
|
||||
For generic functions that operate on arrays or JavaScript objects, you
|
||||
should generally use [Underscore](http://underscorejs.org/). We used to
|
||||
use jQuery's utility functions, but the Underscore equivalents are more
|
||||
consistent, better-behaved and offer more choices.
|
||||
|
||||
A quick conversion table:
|
||||
|
||||
$.each → _.each (parameters to the callback reversed)
|
||||
$.inArray → _.indexOf (parameters reversed)
|
||||
$.grep → _.filter
|
||||
$.map → _.map
|
||||
$.extend → _.extend
|
||||
|
||||
There's a subtle difference in the case of `_.extend`; it will replace
|
||||
attributes with undefined, whereas jQuery won't:
|
||||
|
||||
$.extend({foo: 2}, {foo: undefined}); // yields {foo: 2}, BUT...
|
||||
_.extend({foo: 2}, {foo: undefined}); // yields {foo: undefined}!
|
||||
|
||||
Also, `_.each` does not let you break out of the iteration early by
|
||||
returning false, the way jQuery's version does. If you're doing this,
|
||||
you probably want `_.find`, `_.every`, or `_.any`, rather than 'each'.
|
||||
|
||||
Some Underscore functions have multiple names. You should always use the
|
||||
canonical name (given in large print in the Underscore documentation),
|
||||
with the exception of `_.any`, which we prefer over the less clear
|
||||
'some'.
|
||||
|
||||
More arbitrary style things
|
||||
---------------------------
|
||||
|
||||
### General
|
||||
|
||||
Indentation is four space characters for Python, JS, CSS, and shell
|
||||
scripts. Indentation is two space characters for HTML templates.
|
||||
|
||||
We never use tabs anywhere in source code we write, but we have some
|
||||
third-party files which contain tabs.
|
||||
|
||||
Keep third-party static files under the directory
|
||||
`zephyr/static/third/`, with one subdirectory per third-party project.
|
||||
|
||||
We don't have an absolute hard limit on line length, but we should avoid
|
||||
extremely long lines. A general guideline is: refactor stuff to get it
|
||||
under 85 characters, unless that makes the code a lot uglier, in which
|
||||
case it's fine to go up to 120 or so.
|
||||
|
||||
Whitespace guidelines:
|
||||
|
||||
- Put one space (or more for alignment) around binary arithmetic and
|
||||
equality operators.
|
||||
- Put one space around each part of the ternary operator.
|
||||
- Put one space between keywords like `if` and `while` and their
|
||||
associated open paren.
|
||||
- Put one space between the closing paren for `if` and `while`-like
|
||||
constructs and the opening curly brace. Put the curly brace on the
|
||||
same line unless doing otherwise improves readability.
|
||||
- Put no space before or after the open paren for function calls and
|
||||
no space before the close paren for function calls.
|
||||
- For the comma operator and colon operator in languages where it is
|
||||
used for inline dictionaries, put no space before it and at least
|
||||
one space after. Only use more than one space for alignment.
|
||||
|
||||
### Javascript
|
||||
|
||||
Don't use `==` and `!=` because these operators perform type coercions,
|
||||
which can mask bugs. Always use `===` and `!==`.
|
||||
|
||||
End every statement with a semicolon.
|
||||
|
||||
`if` statements with no braces are allowed, if the body is simple and
|
||||
its extent is abundantly clear from context and formatting.
|
||||
|
||||
Anonymous functions should have spaces before and after the argument
|
||||
list:
|
||||
|
||||
var x = function (foo, bar) { // ...
|
||||
|
||||
When calling a function with an anonymous function as an argument, use
|
||||
this style:
|
||||
|
||||
$.get('foo', function (data) {
|
||||
var x = ...;
|
||||
// ...
|
||||
});
|
||||
|
||||
The inner function body is indented one level from the outer function
|
||||
call. The closing brace for the inner function and the closing
|
||||
parenthesis for the outer call are together on the same line. This style
|
||||
isn't necessarily appropriate for calls with multiple anonymous
|
||||
functions or other arguments following them.
|
||||
|
||||
Use
|
||||
|
||||
$(function () { ...
|
||||
|
||||
rather than
|
||||
|
||||
$(document).ready(function () { ...
|
||||
|
||||
and combine adjacent on-ready functions, if they are logically related.
|
||||
|
||||
The best way to build complicated DOM elements is a Mustache template
|
||||
like `zephyr/static/templates/message.handlebars`. For simpler things
|
||||
you can use jQuery DOM building APIs like so:
|
||||
|
||||
var new_tr = $('<tr />').attr('id', zephyr.id);
|
||||
|
||||
Passing a HTML string to jQuery is fine for simple hardcoded things:
|
||||
|
||||
foo.append('<p id="selected">foo</p>');
|
||||
|
||||
but avoid programmatically building complicated strings.
|
||||
|
||||
We used to favor attaching behaviors in templates like so:
|
||||
|
||||
<p onclick="select_zephyr({{id}})">
|
||||
|
||||
but there are some reasons to prefer attaching events using jQuery code:
|
||||
|
||||
- Potential huge performance gains by using delegated events where
|
||||
possible
|
||||
- When calling a function from an `onclick` attribute, `this` is not
|
||||
bound to the element like you might think
|
||||
- jQuery does event normalization
|
||||
|
||||
Either way, avoid complicated JavaScript code inside HTML attributes;
|
||||
call a helper function instead.
|
||||
|
||||
### HTML / CSS
|
||||
|
||||
Don't use the `style=` attribute. Instead, define logical classes and
|
||||
put your styles in `zulip.css`.
|
||||
|
||||
Don't use the tag name in a selector unless you have to. In other words,
|
||||
use `.foo` instead of `span.foo`. We shouldn't have to care if the tag
|
||||
type changes in the future.
|
||||
|
||||
Don't use inline event handlers (`onclick=`, etc. attributes). Instead,
|
||||
attach a jQuery event handler
|
||||
(`$('#foo').on('click', function () {...})`) when the DOM is ready
|
||||
(inside a `$(function () {...})` block).
|
||||
|
||||
Use this format when you have the same block applying to multiple CSS
|
||||
styles (separate lines for each selector):
|
||||
|
||||
selector1,
|
||||
selector2 {
|
||||
};
|
||||
|
||||
### Python
|
||||
|
||||
- Scripts should start with `#!/usr/bin/env python` and not
|
||||
`#/usr/bin/python` (the right Python may not be installed in
|
||||
`/usr/bin`) or `#/usr/bin/env/python2.7` (bad for Python 3
|
||||
compatibility). Don't put a shebang line on a Python file unless
|
||||
it's meaningful to run it as a script. (Some libraries can also be
|
||||
run as scripts, e.g. to run a test suite.)
|
||||
- The first import in a file should be
|
||||
`from __future__ import absolute_import`, per [PEP
|
||||
328](http://docs.python.org/2/whatsnew/2.5.html#pep-328-absolute-and-relative-imports)
|
||||
- Put all imports together at the top of the file, absent a compelling
|
||||
reason to do otherwise.
|
||||
- Unpacking sequences doesn't require list brackets:
|
||||
|
||||
[x, y] = xs # unnecessary
|
||||
x, y = xs # better
|
||||
|
||||
- For string formatting, use `x % (y,)` rather than `x % y`, to avoid
|
||||
ambiguity if `y` happens to be a tuple.
|
||||
- When selecting by id, don't use `foo.pk` when you mean `foo.id`.
|
||||
E.g.
|
||||
|
||||
recipient = Recipient(type_id=huddle.pk, type=Recipient.HUDDLE)
|
||||
|
||||
should be written as
|
||||
|
||||
recipient = Recipient(type_id=huddle.id, type=Recipient.HUDDLE)
|
||||
|
||||
in case we ever change the primary keys.
|
||||
|
||||
Version Control
|
||||
---------------
|
||||
|
||||
### Commit Discipline
|
||||
|
||||
We follow the Git project's own commit discipline practice of "Each
|
||||
commit is a minimal coherent idea". This discipline takes a bit of work,
|
||||
but it makes it much easier for code reviewers to spot bugs, and
|
||||
makesthe commit history a much more useful resource for developers
|
||||
trying to understand why the code works the way it does, which also
|
||||
helps a lot in preventing bugs.
|
||||
|
||||
Coherency requirements for any commit:
|
||||
|
||||
- It should pass tests (so test updates needed by a change should be
|
||||
in the same commit as the original change, not a separate "fix the
|
||||
tests that were broken by the last commit" commit).
|
||||
- It should be safe to deploy individually, or comment in detail in
|
||||
the commit message as to why it isn't (maybe with a [manual] tag).
|
||||
So implementing a new API endpoint in one commit and then adding the
|
||||
security checks in a future commit should be avoided -- the security
|
||||
checks should be there from the beginning.
|
||||
- Error handling should generally be included along with the code that
|
||||
might trigger the error.
|
||||
- TODO comments should be in the commit that introduces the issue or
|
||||
functionality with further work required.
|
||||
|
||||
When you should be minimal:
|
||||
|
||||
- Significant refactorings should be done in a separate commit from
|
||||
functional changes.
|
||||
- Moving code from one file to another should be done in a separate
|
||||
commits from functional changes or even refactoring within a file.
|
||||
- 2 different refactorings should be done in different commits.
|
||||
- 2 different features should be done in different commits.
|
||||
- If you find yourself writing a commit message that reads like a list
|
||||
of somewhat dissimilar things that you did, you probably should have
|
||||
just done 2 commits.
|
||||
|
||||
When not to be overly minimal:
|
||||
|
||||
- For completely new features, you don't necessarily need to split out
|
||||
new commits for each little subfeature of the new feature. E.g. if
|
||||
you're writing a new tool from scratch, it's fine to have the
|
||||
initial tool have plenty of options/features without doing separate
|
||||
commits for each one. That said, reviewing a 2000-line giant blob of
|
||||
new code isn't fun, so please be thoughtful about submitting things
|
||||
in reviewable units.
|
||||
- Don't bother to split back end commits from front end commits, even
|
||||
though the backend can often be coherent on its own.
|
||||
|
||||
Other considerations:
|
||||
|
||||
- Overly fine commits are easily squashed, but not vice versa, so err
|
||||
toward small commits, and the code reviewer can advise on squashing.
|
||||
- If a commit you write doesn't pass tests, you should usually fix
|
||||
that by amending the commit to fix the bug, not writing a new "fix
|
||||
tests" commit on top of it.
|
||||
|
||||
Zulip expects you to structure the commits in your pull requests to form
|
||||
a clean history before we will merge them; it's best to write your
|
||||
commits following these guidelines in the first place, but if you don't,
|
||||
you can always fix your history using git rebase -i.
|
||||
|
||||
It can take some practice to get used to writing your commits with a
|
||||
clean history so that you don't spend much time doing interactive
|
||||
rebases. For example, often you'll start adding a feature, and discover
|
||||
you need to a refactoring partway through writing the feature. When that
|
||||
happens, we recommend stashing your partial feature, do the refactoring,
|
||||
commit it, and then finish implementing your feature.
|
||||
|
||||
### Commit Messages
|
||||
|
||||
- The first line of commit messages should be written in the
|
||||
imperative and be kept relatively short while concisely explaining
|
||||
what the commit does. For example:
|
||||
|
||||
Bad:
|
||||
|
||||
bugfix
|
||||
gather_subscriptions was broken
|
||||
fix bug #234.
|
||||
|
||||
Good:
|
||||
|
||||
Fix gather_subscriptions throwing an exception when given bad input.
|
||||
|
||||
- Use present-tense action verbs in your commit messages.
|
||||
|
||||
Bad:
|
||||
|
||||
Fixing gather_subscriptions throwing an exception when given bad input.
|
||||
Fixed gather_subscriptions throwing an exception when given bad input.
|
||||
|
||||
Good:
|
||||
|
||||
Fix gather_subscriptions throwing an exception when given bad input.
|
||||
|
||||
- Please use a complete sentence in the summary, ending with a period.
|
||||
- The rest of the commit message should be written in full prose and
|
||||
explain why and how the change was made. If the commit makes
|
||||
performance improvements, you should generally include some rough
|
||||
benchmarks showing that it actually improves the performance.
|
||||
- When you fix a GitHub issue, [mark that you've fixed the issue in
|
||||
your commit
|
||||
message](https://help.github.com/articles/closing-issues-via-commit-messages/)
|
||||
so that the issue is automatically closed when your code is merged.
|
||||
Zulip's preferred style for this is to have the final paragraph of
|
||||
the commit message read e.g. "Fixes: \#123."
|
||||
- Any paragraph content in the commit message should be line-wrapped
|
||||
to less than 76 characters per line, so that your commit message
|
||||
will be reasonably readable in git log in a normal terminal.
|
||||
- In your commit message, you should describe any manual testing you
|
||||
did in addition to running the automated tests, and any aspects of
|
||||
the commit that you think are questionable and you'd like special
|
||||
attention applied to.
|
||||
|
||||
### Tests
|
||||
|
||||
All significant new features should come with tests. See testing.
|
||||
|
||||
### Third party code
|
||||
|
||||
When adding new third-party packages to our codebase, please include
|
||||
"[third]" at the beginning of the commit message. You don't necessarily
|
||||
need to do this when patching third-party code that's already in tree.
|
||||
@@ -1,524 +0,0 @@
|
||||
==========================
|
||||
Code style and conventions
|
||||
==========================
|
||||
|
||||
Be consistent!
|
||||
==============
|
||||
|
||||
Look at the surrounding code, or a similar part of the project, and
|
||||
try to do the same thing. If you think the other code has actively bad
|
||||
style, fix it (in a separate commit).
|
||||
|
||||
When in doubt, send an email to zulip-devel@googlegroups.com with your
|
||||
question.
|
||||
|
||||
Lint tools
|
||||
==========
|
||||
|
||||
You can run them all at once with
|
||||
|
||||
::
|
||||
|
||||
./tools/lint-all
|
||||
|
||||
You can set this up as a local Git commit hook with
|
||||
|
||||
::
|
||||
|
||||
``tools/setup-git-repo``
|
||||
|
||||
The Vagrant setup process runs this for you.
|
||||
|
||||
``lint-all`` runs many lint checks in parallel, including
|
||||
|
||||
- Javascript (`JSLint <http://www.jslint.com/>`__)
|
||||
|
||||
``tools/jslint/check-all.js`` contains a pretty fine-grained set of
|
||||
JSLint options, rule exceptions, and allowed global variables. If you
|
||||
add a new global, you'll need to add it to the list.
|
||||
|
||||
- Python (`Pyflakes <http://pypi.python.org/pypi/pyflakes>`__)
|
||||
- templates
|
||||
- Puppet configuration
|
||||
- custom checks (e.g. trailing whitespace and spaces-not-tabs)
|
||||
|
||||
Secrets
|
||||
=======
|
||||
|
||||
Please don't put any passwords, secret access keys, etc. inline in the
|
||||
code. Instead, use the ``get_secret`` function in
|
||||
``zproject/settings.py`` to read secrets from ``/etc/zulip/secrets.conf``.
|
||||
|
||||
Dangerous constructs
|
||||
====================
|
||||
|
||||
Misuse of database queries
|
||||
--------------------------
|
||||
|
||||
Look out for Django code like this::
|
||||
|
||||
[Foo.objects.get(id=bar.x.id)
|
||||
for bar in Bar.objects.filter(...)
|
||||
if bar.baz < 7]
|
||||
|
||||
This will make one database query for each ``Bar``, which is slow in
|
||||
production (but not in local testing!). Instead of a list comprehension,
|
||||
write a single query using Django's `QuerySet
|
||||
API <https://docs.djangoproject.com/en/dev/ref/models/querysets/>`__.
|
||||
|
||||
If you can't rewrite it as a single query, that's a sign that something
|
||||
is wrong with the database schema. So don't defer this optimization when
|
||||
performing schema changes, or else you may later find that it's
|
||||
impossible.
|
||||
|
||||
UserProfile.objects.get() / Client.objects.get / etc.
|
||||
-----------------------------------------------------
|
||||
|
||||
In our Django code, never do direct
|
||||
``UserProfile.objects.get(email=foo)`` database queries. Instead always
|
||||
use ``get_user_profile_by_{email,id}``. There are 3 reasons for this:
|
||||
|
||||
#. It's guaranteed to correctly do a case-inexact lookup
|
||||
#. It fetches the user object from remote cache, which is faster
|
||||
#. It always fetches a UserProfile object which has been queried using
|
||||
.selected\_related(), and thus will perform well when one later
|
||||
accesses related models like the Realm.
|
||||
|
||||
Similarly we have ``get_client`` and ``get_stream`` functions to fetch
|
||||
those commonly accessed objects via remote cache.
|
||||
|
||||
Using Django model objects as keys in sets/dicts
|
||||
------------------------------------------------
|
||||
|
||||
Don't use Django model objects as keys in sets/dictionaries -- you will
|
||||
get unexpected behavior when dealing with objects obtained from
|
||||
different database queries:
|
||||
|
||||
For example,
|
||||
``UserProfile.objects.only("id").get(id=17) in set([UserProfile.objects.get(id=17)])``
|
||||
is False
|
||||
|
||||
You should work with the IDs instead.
|
||||
|
||||
user\_profile.save()
|
||||
--------------------
|
||||
|
||||
You should always pass the update\_fields keyword argument to .save()
|
||||
when modifying an existing Django model object. By default, .save() will
|
||||
overwrite every value in the column, which results in lots of race
|
||||
conditions where unrelated changes made by one thread can be
|
||||
accidentally overwritten by another thread that fetched its UserProfile
|
||||
object before the first thread wrote out its change.
|
||||
|
||||
Using raw saves to update important model objects
|
||||
-------------------------------------------------
|
||||
|
||||
In most cases, we already have a function in zephyr/lib/actions.py with
|
||||
a name like do\_activate\_user that will correctly handle lookups,
|
||||
caching, and notifying running browsers via the event system about your
|
||||
change. So please check whether such a function exists before writing
|
||||
new code to modify a model object, since your new code has a good chance
|
||||
of getting at least one of these things wrong.
|
||||
|
||||
``x.attr('zid')`` vs. ``rows.id(x)``
|
||||
------------------------------------
|
||||
|
||||
Our message row DOM elements have a custom attribute ``zid`` which
|
||||
contains the numerical message ID. **Don't access this directly as**
|
||||
``x.attr('zid')`` ! The result will be a string and comparisons (e.g.
|
||||
with ``<=``) will give the wrong result, occasionally, just enough to
|
||||
make a bug that's impossible to track down.
|
||||
|
||||
You should instead use the ``id`` function from the ``rows`` module, as
|
||||
in ``rows.id(x)``. This returns a number. Even in cases where you do
|
||||
want a string, use the ``id`` function, as it will simplify future code
|
||||
changes. In most contexts in JavaScript where a string is needed, you
|
||||
can pass a number without any explicit conversion.
|
||||
|
||||
Javascript var
|
||||
--------------
|
||||
|
||||
Always declare Javascript variables using ``var``::
|
||||
|
||||
var x = ...;
|
||||
|
||||
In a function, ``var`` is necessary or else ``x`` will be a global
|
||||
variable. For variables declared at global scope, this has no effect,
|
||||
but we do it for consistency.
|
||||
|
||||
Javascript has function scope only, not block scope. This means that a
|
||||
``var`` declaration inside a ``for`` or ``if`` acts the same as a
|
||||
``var`` declaration at the beginning of the surrounding ``function``. To
|
||||
avoid confusion, declare all variables at the top of a function.
|
||||
|
||||
Javascript ``for (i in myArray)``
|
||||
---------------------------------
|
||||
|
||||
Don't use it:
|
||||
`[1] <http://stackoverflow.com/questions/500504/javascript-for-in-with-arrays>`__,
|
||||
`[2] <http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml#for-in_loop>`__,
|
||||
`[3] <http://www.jslint.com/lint.html#forin>`__
|
||||
|
||||
jQuery global state
|
||||
-------------------
|
||||
|
||||
Don't mess with jQuery global state once the app has loaded. Code like
|
||||
this is very dangerous::
|
||||
|
||||
$.ajaxSetup({ async: false });
|
||||
$.get(...);
|
||||
$.ajaxSetup({ async: true });
|
||||
|
||||
jQuery and the browser are free to run other code while the request is
|
||||
pending, which could perform other Ajax requests with the altered
|
||||
settings.
|
||||
|
||||
Instead, switch to the more general |ajax|_ function, which can take options
|
||||
like ``async``.
|
||||
|
||||
.. |ajax| replace:: ``$.ajax``
|
||||
.. _ajax: http://api.jquery.com/jQuery.ajax
|
||||
|
||||
State and logs files
|
||||
--------------------
|
||||
|
||||
Do not write state and logs files inside the current working directory
|
||||
in the production environment. This will not how you expect, because the
|
||||
current working directory for the app changes every time we do a deploy.
|
||||
Instead, hardcode a path in settings.py -- see SERVER\_LOG\_PATH in
|
||||
settings.py for an example.
|
||||
|
||||
JS array/object manipulation
|
||||
============================
|
||||
|
||||
For generic functions that operate on arrays or JavaScript objects, you
|
||||
should generally use `Underscore <http://underscorejs.org/>`__. We used
|
||||
to use jQuery's utility functions, but the Underscore equivalents are
|
||||
more consistent, better-behaved and offer more choices.
|
||||
|
||||
A quick conversion table::
|
||||
|
||||
$.each → _.each (parameters to the callback reversed)
|
||||
$.inArray → _.indexOf (parameters reversed)
|
||||
$.grep → _.filter
|
||||
$.map → _.map
|
||||
$.extend → _.extend
|
||||
|
||||
There's a subtle difference in the case of ``_.extend``; it will replace
|
||||
attributes with undefined, whereas jQuery won't::
|
||||
|
||||
$.extend({foo: 2}, {foo: undefined}); // yields {foo: 2}, BUT...
|
||||
_.extend({foo: 2}, {foo: undefined}); // yields {foo: undefined}!
|
||||
|
||||
Also, ``_.each`` does not let you break out of the iteration early by
|
||||
returning false, the way jQuery's version does. If you're doing this,
|
||||
you probably want ``_.find``, ``_.every``, or ``_.any``, rather than
|
||||
'each'.
|
||||
|
||||
Some Underscore functions have multiple names. You should always use the
|
||||
canonical name (given in large print in the Underscore documentation),
|
||||
with the exception of ``_.any``, which we prefer over the less clear
|
||||
'some'.
|
||||
|
||||
More arbitrary style things
|
||||
===========================
|
||||
|
||||
General
|
||||
-------
|
||||
|
||||
Indentation is four space characters for Python, JS, CSS, and shell
|
||||
scripts. Indentation is two space characters for HTML templates.
|
||||
|
||||
We never use tabs anywhere in source code we write, but we have some
|
||||
third-party files which contain tabs.
|
||||
|
||||
Keep third-party static files under the directory
|
||||
``zephyr/static/third/``, with one subdirectory per third-party project.
|
||||
|
||||
We don't have an absolute hard limit on line length, but we should avoid
|
||||
extremely long lines. A general guideline is: refactor stuff to get it
|
||||
under 85 characters, unless that makes the code a lot uglier, in which
|
||||
case it's fine to go up to 120 or so.
|
||||
|
||||
Whitespace guidelines:
|
||||
|
||||
- Put one space (or more for alignment) around binary arithmetic and
|
||||
equality operators.
|
||||
- Put one space around each part of the ternary operator.
|
||||
- Put one space between keywords like ``if`` and ``while`` and their
|
||||
associated open paren.
|
||||
- Put one space between the closing paren for ``if`` and ``while``-like
|
||||
constructs and the opening curly brace. Put the curly brace on the
|
||||
same line unless doing otherwise improves readability.
|
||||
- Put no space before or after the open paren for function calls and no
|
||||
space before the close paren for function calls.
|
||||
- For the comma operator and colon operator in languages where it is
|
||||
used for inline dictionaries, put no space before it and at least one
|
||||
space after. Only use more than one space for alignment.
|
||||
|
||||
Javascript
|
||||
----------
|
||||
|
||||
Don't use ``==`` and ``!=`` because these operators perform type
|
||||
coercions, which can mask bugs. Always use ``===`` and ``!==``.
|
||||
|
||||
End every statement with a semicolon.
|
||||
|
||||
``if`` statements with no braces are allowed, if the body is simple and
|
||||
its extent is abundantly clear from context and formatting.
|
||||
|
||||
Anonymous functions should have spaces before and after the argument
|
||||
list::
|
||||
|
||||
var x = function (foo, bar) { // ...
|
||||
|
||||
When calling a function with an anonymous function as an argument, use
|
||||
this style::
|
||||
|
||||
$.get('foo', function (data) {
|
||||
var x = ...;
|
||||
// ...
|
||||
});
|
||||
|
||||
The inner function body is indented one level from the outer function
|
||||
call. The closing brace for the inner function and the closing
|
||||
parenthesis for the outer call are together on the same line. This style
|
||||
isn't necessarily appropriate for calls with multiple anonymous
|
||||
functions or other arguments following them.
|
||||
|
||||
Use
|
||||
|
||||
::
|
||||
|
||||
$(function () { ...
|
||||
|
||||
rather than
|
||||
|
||||
::
|
||||
|
||||
$(document).ready(function () { ...
|
||||
|
||||
and combine adjacent on-ready functions, if they are logically related.
|
||||
|
||||
The best way to build complicated DOM elements is a Mustache template
|
||||
like ``zephyr/static/templates/message.handlebars``. For simpler things
|
||||
you can use jQuery DOM building APIs like so::
|
||||
|
||||
var new_tr = $('<tr />').attr('id', zephyr.id);
|
||||
|
||||
Passing a HTML string to jQuery is fine for simple hardcoded things::
|
||||
|
||||
foo.append('<p id="selected">foo</p>');
|
||||
|
||||
but avoid programmatically building complicated strings.
|
||||
|
||||
We used to favor attaching behaviors in templates like so::
|
||||
|
||||
<p onclick="select_zephyr({{id}})">
|
||||
|
||||
but there are some reasons to prefer attaching events using jQuery code:
|
||||
|
||||
- Potential huge performance gains by using delegated events where
|
||||
possible
|
||||
- When calling a function from an ``onclick`` attribute, ``this`` is
|
||||
not bound to the element like you might think
|
||||
- jQuery does event normalization
|
||||
|
||||
Either way, avoid complicated JavaScript code inside HTML attributes;
|
||||
call a helper function instead.
|
||||
|
||||
HTML / CSS
|
||||
----------
|
||||
|
||||
Don't use the ``style=`` attribute. Instead, define logical classes and
|
||||
put your styles in ``zulip.css``.
|
||||
|
||||
Don't use the tag name in a selector unless you have to. In other words,
|
||||
use ``.foo`` instead of ``span.foo``. We shouldn't have to care if the
|
||||
tag type changes in the future.
|
||||
|
||||
Don't use inline event handlers (``onclick=``, etc. attributes).
|
||||
Instead, attach a jQuery event handler
|
||||
(``$('#foo').on('click', function () {...})``) when the DOM is ready
|
||||
(inside a ``$(function () {...})`` block).
|
||||
|
||||
Use this format when you have the same block applying to multiple CSS
|
||||
styles (separate lines for each selector)::
|
||||
|
||||
selector1,
|
||||
selector2 {
|
||||
};
|
||||
|
||||
Python
|
||||
------
|
||||
|
||||
- Scripts should start with ``#!/usr/bin/env python`` and not
|
||||
``#/usr/bin/python`` (the right Python may not be installed at
|
||||
/usr/bin) or ``#/usr/bin/env/python2.7`` (bad for Python 3
|
||||
compatibility). Don't put a shebang line on a Python file unless
|
||||
it's meaningful to run it as a script. (Some libraries can also be
|
||||
run as scripts, e.g. to run a test suite.)
|
||||
|
||||
- The first import in a file should be
|
||||
``from __future__ import absolute_import``, per `PEP
|
||||
328 <http://docs.python.org/2/whatsnew/2.5.html#pep-328-absolute-and-relative-imports>`__
|
||||
- Put all imports together at the top of the file, absent a compelling
|
||||
reason to do otherwise.
|
||||
- Unpacking sequences doesn't require list brackets::
|
||||
|
||||
[x, y] = xs # unnecessary
|
||||
x, y = xs # better
|
||||
|
||||
- For string formatting, use ``x % (y,)`` rather than ``x % y``, to
|
||||
avoid ambiguity if ``y`` happens to be a tuple.
|
||||
- When selecting by id, don't use ``foo.pk`` when you mean ``foo.id``.
|
||||
E.g.
|
||||
|
||||
::
|
||||
|
||||
recipient = Recipient(type_id=huddle.pk, type=Recipient.HUDDLE)
|
||||
|
||||
should be written as
|
||||
|
||||
::
|
||||
|
||||
recipient = Recipient(type_id=huddle.id, type=Recipient.HUDDLE)
|
||||
|
||||
in case we ever change the primary keys.
|
||||
|
||||
Version Control
|
||||
===============
|
||||
|
||||
Commit Discipline
|
||||
-----------------
|
||||
|
||||
We follow the Git project's own commit discipline practice of "Each
|
||||
commit is a minimal coherent idea". This discipline takes a bit of
|
||||
work, but it makes it much easier for code reviewers to spot bugs, and
|
||||
makesthe commit history a much more useful resource for developers
|
||||
trying to understand why the code works the way it does, which also
|
||||
helps a lot in preventing bugs.
|
||||
|
||||
Coherency requirements for any commit:
|
||||
|
||||
- It should pass tests (so test updates needed by a change should be in
|
||||
the same commit as the original change, not a separate "fix the tests
|
||||
that were broken by the last commit" commit).
|
||||
- It should be safe to deploy individually, or comment in detail in the
|
||||
commit message as to why it isn't (maybe with a [manual] tag). So
|
||||
implementing a new API endpoint in one commit and then adding the
|
||||
security checks in a future commit should be avoided -- the security
|
||||
checks should be there from the beginning.
|
||||
- Error handling should generally be included along with the code that
|
||||
might trigger the error.
|
||||
- TODO comments should be in the commit that introduces the
|
||||
issue or functionality with further work required.
|
||||
|
||||
When you should be minimal:
|
||||
|
||||
- Significant refactorings should be done in a separate commit from
|
||||
functional changes.
|
||||
- Moving code from one file to another should be done in a separate
|
||||
commits from functional changes or even refactoring within a file.
|
||||
- 2 different refactorings should be done in different commits.
|
||||
- 2 different features should be done in different commits.
|
||||
- If you find yourself writing a commit message that reads like a list
|
||||
of somewhat dissimilar things that you did, you probably should have
|
||||
just done 2 commits.
|
||||
|
||||
When not to be overly minimal:
|
||||
|
||||
- For completely new features, you don't necessarily need to split out
|
||||
new commits for each little subfeature of the new feature. E.g. if
|
||||
you're writing a new tool from scratch, it's fine to have the initial
|
||||
tool have plenty of options/features without doing separate commits
|
||||
for each one. That said, reviewing a 2000-line giant blob of new
|
||||
code isn't fun, so please be thoughtful about submitting things in
|
||||
reviewable units.
|
||||
- Don't bother to split back end commits from front end commits, even
|
||||
though the backend can often be coherent on its own.
|
||||
|
||||
Other considerations:
|
||||
|
||||
- Overly fine commits are easily squashed, but not vice versa, so err
|
||||
toward small commits, and the code reviewer can advise on squashing.
|
||||
- If a commit you write doesn't pass tests, you should usually fix
|
||||
that by amending the commit to fix the bug, not writing a new "fix
|
||||
tests" commit on top of it.
|
||||
|
||||
Zulip expects you to structure the commits in your pull requests to
|
||||
form a clean history before we will merge them; it's best to write
|
||||
your commits following these guidelines in the first place, but if you
|
||||
don't, you can always fix your history using `git rebase -i`.
|
||||
|
||||
It can take some practice to get used to writing your commits with a
|
||||
clean history so that you don't spend much time doing interactive
|
||||
rebases. For example, often you'll start adding a feature, and
|
||||
discover you need to a refactoring partway through writing the
|
||||
feature. When that happens, we recommend stashing your partial
|
||||
feature, do the refactoring, commit it, and then finish implementing
|
||||
your feature.
|
||||
|
||||
Commit Messages
|
||||
---------------
|
||||
|
||||
- The first line of commit messages should be written in the imperative
|
||||
and be kept relatively short while concisely explaining what the
|
||||
commit does. For example:
|
||||
|
||||
Bad::
|
||||
|
||||
bugfix
|
||||
gather_subscriptions was broken
|
||||
fix bug #234.
|
||||
|
||||
Good::
|
||||
|
||||
Fix gather_subscriptions throwing an exception when given bad input.
|
||||
|
||||
- Use present-tense action verbs in your commit messages.
|
||||
|
||||
Bad::
|
||||
|
||||
Fixing gather_subscriptions throwing an exception when given bad input.
|
||||
Fixed gather_subscriptions throwing an exception when given bad input.
|
||||
|
||||
Good::
|
||||
|
||||
Fix gather_subscriptions throwing an exception when given bad input.
|
||||
|
||||
- Please use a complete sentence in the summary, ending with a
|
||||
period.
|
||||
|
||||
- The rest of the commit message should be written in full prose and
|
||||
explain why and how the change was made. If the commit makes
|
||||
performance improvements, you should generally include some rough
|
||||
benchmarks showing that it actually improves the performance.
|
||||
|
||||
- When you fix a GitHub issue, `mark that you've fixed the issue in
|
||||
your commit message
|
||||
<https://help.github.com/articles/closing-issues-via-commit-messages/>`__
|
||||
so that the issue is automatically closed when your code is merged.
|
||||
Zulip's preferred style for this is to have the final paragraph
|
||||
of the commit message read e.g. "Fixes: #123."
|
||||
|
||||
- Any paragraph content in the commit message should be line-wrapped
|
||||
to less than 76 characters per line, so that your commit message
|
||||
will be reasonably readable in `git log` in a normal terminal.
|
||||
|
||||
- In your commit message, you should describe any manual testing you
|
||||
did in addition to running the automated tests, and any aspects of
|
||||
the commit that you think are questionable and you'd like special
|
||||
attention applied to.
|
||||
|
||||
Tests
|
||||
-----
|
||||
|
||||
All significant new features should come with tests. See :doc:`testing`.
|
||||
|
||||
Third party code
|
||||
----------------
|
||||
|
||||
When adding new third-party packages to our codebase, please include
|
||||
"[third]" at the beginning of the commit message. You don't necessarily
|
||||
need to do this when patching third-party code that's already in tree.
|
||||
115
docs/directory-structure.md
Normal file
115
docs/directory-structure.md
Normal file
@@ -0,0 +1,115 @@
|
||||
Directory structure
|
||||
===================
|
||||
|
||||
This page documents the Zulip directory structure and how to decide
|
||||
where to put a file.
|
||||
|
||||
Scripts
|
||||
-------
|
||||
|
||||
* `scripts/` Scripts that production deployments might run manually
|
||||
(e.g., `restart-server`).
|
||||
|
||||
* `scripts/lib/` Scripts that are needed on production deployments but
|
||||
humans should never run.
|
||||
|
||||
* `scripts/setup/` Tools that production deployments will only run
|
||||
once, during installation.
|
||||
|
||||
* `tools/` Development tools.
|
||||
|
||||
---------------------------------------------------------
|
||||
|
||||
Bots
|
||||
----
|
||||
|
||||
* `api/integrations/` Bots distributed as part of the Zulip API bundle.
|
||||
|
||||
* `bots/` Previously Zulip internal bots. These usually need a bit of
|
||||
work.
|
||||
|
||||
-----------------------------------------------------
|
||||
|
||||
Management commands
|
||||
-------------------
|
||||
|
||||
* `zerver/management/commands/` Management commands one might run at a
|
||||
production deployment site (e.g. scripts to change a value or
|
||||
deactivate a user properly)
|
||||
|
||||
-------------------------------------------------------------------------
|
||||
|
||||
Views
|
||||
-----
|
||||
|
||||
* `zerver/tornadoviews.py` Tornado views
|
||||
|
||||
* `zerver/views/webhooks.py` Webhook views
|
||||
|
||||
* `zerver/views/messages.py` message-related views
|
||||
|
||||
* `zerver/views/__init__.py` other Django views
|
||||
|
||||
----------------------------------------
|
||||
|
||||
Jinja2 Compatibility Files
|
||||
--------------------------
|
||||
|
||||
* `zproject/jinja2/__init__.py` Jinja2 environment
|
||||
|
||||
* `zproject/jinja2/backends.py` Jinja2 backend
|
||||
|
||||
* `zproject/jinja2/compressors.py` Jinja2 compatible functions of
|
||||
Django-Pipeline
|
||||
|
||||
-----------------------------------------------------------------------
|
||||
|
||||
Static assets
|
||||
-------------
|
||||
|
||||
* `assets/` For assets not to be served to the web (e.g. the system to
|
||||
generate our favicons)
|
||||
|
||||
* `static/` For things we do want to both serve to the web and
|
||||
distribute to production deployments (e.g. the webpages)
|
||||
|
||||
---------------------------------------------------------------
|
||||
|
||||
Puppet
|
||||
------
|
||||
|
||||
* `puppet/zulip/` For configuration for production deployments
|
||||
|
||||
-------------------------------------------------------------------
|
||||
|
||||
Templates
|
||||
---------
|
||||
|
||||
* `templates/zerver/` For Jinja2 templates for the backend (for zerver app)
|
||||
|
||||
* `static/templates/` Handlebars templates for the frontend
|
||||
|
||||
-----------------------------------------------------------------------
|
||||
|
||||
Tests
|
||||
-----
|
||||
|
||||
* `zerver/tests/` Backend tests
|
||||
|
||||
* `frontend_tests/node_tests/` Node Frontend unit tests
|
||||
|
||||
* `frontend_tests/casper_tests/` Casper frontend tests
|
||||
|
||||
-----------------------------------------------------------------------
|
||||
|
||||
|
||||
Documentation
|
||||
-------------
|
||||
|
||||
* `docs/` Source for this documentation
|
||||
|
||||
--------------------------------------------------------------
|
||||
|
||||
You can consult the repository's `.gitattributes` file to see exactly
|
||||
which components are excluded from production releases (release
|
||||
tarballs are generated using `tools/build-release-tarball`).
|
||||
@@ -1,107 +0,0 @@
|
||||
===================
|
||||
Directory structure
|
||||
===================
|
||||
|
||||
This page documents the Zulip directory structure and how to decide where to
|
||||
put a file.
|
||||
|
||||
Scripts
|
||||
=======
|
||||
|
||||
+--------------------+-----------------------------------------------------------------------------------+
|
||||
| ``scripts/`` | Scripts that production deployments might run manually (e.g. ``restart-server``) |
|
||||
+--------------------+-----------------------------------------------------------------------------------+
|
||||
| ``scripts/lib/`` | Scripts that are needed on production deployments but humans should never run |
|
||||
+--------------------+-----------------------------------------------------------------------------------+
|
||||
| ``scripts/setup/`` | Tools that production deployments will only run once, during installation |
|
||||
+--------------------+-----------------------------------------------------------------------------------+
|
||||
| ``tools/`` | Development tools |
|
||||
+--------------------+-----------------------------------------------------------------------------------+
|
||||
|
||||
Bots
|
||||
====
|
||||
|
||||
+------------------------+----------------------------------------------------------------------+
|
||||
| ``api/integrations`` | Bots distributed as part of the Zulip API bundle. |
|
||||
+------------------------+----------------------------------------------------------------------+
|
||||
| ``bots/`` | Previously Zulip internal bots. These usually need a bit of work. |
|
||||
+------------------------+----------------------------------------------------------------------+
|
||||
|
||||
Management commands
|
||||
===================
|
||||
|
||||
+-------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
|
||||
| ``zerver/management/commands/`` | Management commands one might run at a production deployment site (e.g. scripts to change a value or deactivate a user properly) |
|
||||
+-------------------------------------+------------------------------------------------------------------------------------------------------------------------------------+
|
||||
|
||||
Views
|
||||
=====
|
||||
|
||||
+--------------------------------+-----------------------------------------+
|
||||
| ``zerver/tornadoviews.py`` | Tornado views |
|
||||
+--------------------------------+-----------------------------------------+
|
||||
| ``zerver/views/webhooks.py`` | Webhook views |
|
||||
+--------------------------------+-----------------------------------------+
|
||||
| ``zerver/views/messages.py`` | message-related views |
|
||||
+--------------------------------+-----------------------------------------+
|
||||
| ``zerver/views/__init__.py`` | other Django views |
|
||||
+--------------------------------+-----------------------------------------+
|
||||
|
||||
Jinja2 Compatibility Files
|
||||
==========================
|
||||
|
||||
+-------------------------------------+--------------------------------------------------------------------+
|
||||
| ``zproject/jinja2/__init__.py`` | Jinja2 environment |
|
||||
+-------------------------------------+--------------------------------------------------------------------+
|
||||
| ``zproject/jinja2/backends.py`` | Jinja2 backend |
|
||||
+-------------------------------------+--------------------------------------------------------------------+
|
||||
| ``zproject/jinja2/compressors.py`` | Jinja2 compatible functions of Django-Pipeline |
|
||||
+-------------------------------------+--------------------------------------------------------------------+
|
||||
|
||||
|
||||
Static assets
|
||||
=============
|
||||
|
||||
+---------------+---------------------------------------------------------------------------------------------------------------+
|
||||
| ``assets/`` | For assets not to be served to the web (e.g. the system to generate our favicons) |
|
||||
+---------------+---------------------------------------------------------------------------------------------------------------+
|
||||
| ``static/`` | For things we do want to both serve to the web and distribute to production deployments (e.g. the webpages) |
|
||||
+---------------+---------------------------------------------------------------------------------------------------------------+
|
||||
|
||||
Puppet
|
||||
======
|
||||
|
||||
+--------------------+----------------------------------------------------------------------------------+
|
||||
| ``puppet/zulip`` | For configuration for production deployments |
|
||||
+--------------------+----------------------------------------------------------------------------------+
|
||||
|
||||
Templates
|
||||
=========
|
||||
|
||||
+--------------------------+--------------------------------------------------------+
|
||||
| ``templates/zerver`` | For Jinja2 templates for the backend (for zerver app) |
|
||||
+--------------------------+--------------------------------------------------------+
|
||||
| ``static/templates`` | Handlebars templates for the frontend |
|
||||
+--------------------------+--------------------------------------------------------+
|
||||
|
||||
Tests
|
||||
=====
|
||||
|
||||
+-------------------------+-----------------------------------+
|
||||
| ``zerver/tests/`` | Backend tests |
|
||||
+-------------------------+-----------------------------------+
|
||||
| ``frontend_tests/node`` | Node Frontend unit tests |
|
||||
+-------------------------+-----------------------------------+
|
||||
| ``frontend_tests/tests``| Casper frontend tests |
|
||||
+-------------------------+-----------------------------------+
|
||||
|
||||
Documentation
|
||||
=============
|
||||
|
||||
+-------------+-----------------------------------------------+
|
||||
| ``docs/`` | Source for this documentation |
|
||||
+-------------+-----------------------------------------------+
|
||||
|
||||
You can consult the repository's .gitattributes file to see exactly
|
||||
which components are excluded from production releases (release
|
||||
tarballs are generated using tools/build-release-tarball).
|
||||
73
docs/front-end-build-process.md
Normal file
73
docs/front-end-build-process.md
Normal file
@@ -0,0 +1,73 @@
|
||||
Front End Build Process
|
||||
=======================
|
||||
|
||||
This page documents additional information that may be useful when
|
||||
developing new features for Zulip that require front-end changes. For a
|
||||
more general overview, see the new-feature-tutorial. The code-style
|
||||
documentation also has relevant information about how Zulip's code is
|
||||
structured.
|
||||
|
||||
Primary build process
|
||||
---------------------
|
||||
|
||||
Most of the existing JS in Zulip is written in IIFE-wrapped modules, one
|
||||
per file in the static/js directory. When running Zulip in development
|
||||
mode, each file is loaded seperately. In production mode (and when
|
||||
creating a release tarball using tools/build-release-tarball),
|
||||
JavaScript files are concatenated and minified.
|
||||
|
||||
If you add a new JavaScript file, it needs to be specified in the
|
||||
JS\_SPECS dictionary defined in zproject/settings.py to be included in
|
||||
the concatenated file.
|
||||
|
||||
Webpack/CommonJS modules
|
||||
------------------------
|
||||
|
||||
New JS written for Zulip can be written as CommonJS modules (bundled
|
||||
using [webpack](https://webpack.github.io/), though this will taken care
|
||||
of automatically whenever `run-dev.py` is running). (CommonJS is the
|
||||
same module format that Node uses, so see the [Node
|
||||
documentation](https://nodejs.org/docs/latest/api/modules.html) for
|
||||
more information on the syntax.)
|
||||
|
||||
Benefits of using CommonJS modules over the
|
||||
[IIFE](http://benalman.com/news/2010/11/immediately-invoked-function-expression/)
|
||||
module approach:
|
||||
|
||||
- namespacing/module boilerplate will be added automatically in the
|
||||
bundling process
|
||||
- dependencies between modules are more explicit and easier to trace
|
||||
- no separate list of JS files needs to be maintained for
|
||||
concatenation and minification
|
||||
- third-party libraries can be more easily installed/versioned using
|
||||
npm
|
||||
- running the same code in the browser and in Node for testing is
|
||||
simplified (as both environments use the same module syntax)
|
||||
|
||||
The entry point file for the bundle generated by webpack is
|
||||
`static/js/src/main.js`. Any modules you add will need to be required
|
||||
from this file (or one of its dependencies) in order to be included in
|
||||
the script bundle.
|
||||
|
||||
Adding static files
|
||||
-------------------
|
||||
|
||||
To add a static file to the app (JavaScript, CSS, images, etc), first
|
||||
add it to the appropriate place under `static/`.
|
||||
|
||||
- Third-party files should all go in `static/third/`. Tag the commit
|
||||
with "[third]" when adding or modifying a third-party package.
|
||||
- Our own JS lives under `static/js`; CSS lives under `static/styles`.
|
||||
- JavaScript and CSS files are combined and minified in production. In
|
||||
this case all you need to do is add the filename to PIPELINE\_CSS or
|
||||
JS\_SPECS in `zproject/settings.py`. (If you plan to only use the
|
||||
JS/CSS within the app proper, and not on the login page or other
|
||||
standalone pages, put it in the 'app' category.)
|
||||
|
||||
If you want to test minified files in development, look for the
|
||||
`PIPELINE =` line in `zproject/settings.py` and set it to `True` -- or
|
||||
just set `DEBUG = False`.
|
||||
|
||||
Note that `static/html/{400,5xx}.html` will only render properly if
|
||||
minification is enabled, since they hardcode the path
|
||||
`static/min/portico.css`.
|
||||
@@ -1,75 +0,0 @@
|
||||
=======================
|
||||
Front End Build Process
|
||||
=======================
|
||||
|
||||
This page documents additional information that may be useful when
|
||||
developing new features for Zulip that require front-end changes. For
|
||||
a more general overview, see the :doc:`new-feature-tutorial`. The
|
||||
:doc:`code-style` documentation also has relevant information about
|
||||
how Zulip's code is structured.
|
||||
|
||||
Primary build process
|
||||
=====================
|
||||
|
||||
Most of the existing JS in Zulip is written in IIFE-wrapped modules,
|
||||
one per file in the `static/js` directory. When running Zulip in
|
||||
development mode, each file is loaded seperately. In production mode
|
||||
(and when creating a release tarball using
|
||||
`tools/build-release-tarball`), JavaScript files are concatenated and
|
||||
minified.
|
||||
|
||||
If you add a new JavaScript file, it needs to be specified in the
|
||||
`JS_SPECS` dictionary defined in `zproject/settings.py` to be included
|
||||
in the concatenated file.
|
||||
|
||||
Webpack/CommonJS modules
|
||||
========================
|
||||
|
||||
New JS written for Zulip can be written as CommonJS modules (bundled
|
||||
using `webpack <https://webpack.github.io/>`_, though this will taken
|
||||
care of automatically whenever ``run-dev.py`` is running). (CommonJS
|
||||
is the same module format that Node uses, so see `the Node
|
||||
documentation <https://nodejs.org/docs/latest/api/modules.html>` for
|
||||
more information on the syntax.)
|
||||
|
||||
Benefits of using CommonJS modules over the `IIFE
|
||||
<http://benalman.com/news/2010/11/immediately-invoked-function-expression/>`_
|
||||
module approach:
|
||||
|
||||
* namespacing/module boilerplate will be added automatically in the bundling process
|
||||
* dependencies between modules are more explicit and easier to trace
|
||||
* no separate list of JS files needs to be maintained for concatenation and minification
|
||||
* third-party libraries can be more easily installed/versioned using npm
|
||||
* running the same code in the browser and in Node for testing is
|
||||
simplified (as both environments use the same module syntax)
|
||||
|
||||
The entry point file for the bundle generated by webpack is
|
||||
``static/js/src/main.js``. Any modules you add will need to be
|
||||
required from this file (or one of its dependencies) in order to be
|
||||
included in the script bundle.
|
||||
|
||||
Adding static files
|
||||
===================
|
||||
|
||||
To add a static file to the app (JavaScript, CSS, images, etc), first
|
||||
add it to the appropriate place under ``static/``.
|
||||
|
||||
* Third-party files should all go in ``static/third/``. Tag the commit
|
||||
with "[third]" when adding or modifying a third-party package.
|
||||
|
||||
* Our own JS lives under ``static/js``; CSS lives under ``static/styles``.
|
||||
|
||||
* JavaScript and CSS files are combined and minified in production. In
|
||||
this case all you need to do is add the filename to PIPELINE_CSS or
|
||||
JS_SPECS in ``zproject/settings.py``. (If you plan to only use the
|
||||
JS/CSS within the app proper, and not on the login page or other
|
||||
standalone pages, put it in the 'app' category.)
|
||||
|
||||
If you want to test minified files in development, look for the
|
||||
``PIPELINE =`` line in ``zproject/settings.py`` and set it to ``True`` -- or
|
||||
just set ``DEBUG = False``.
|
||||
|
||||
Note that ``static/html/{400,5xx}.html`` will only render properly if
|
||||
minification is enabled, since they hardcode the path
|
||||
``static/min/portico.css``.
|
||||
|
||||
@@ -21,7 +21,7 @@ products, ordered here by which types we prefer to write:
|
||||
third-party service supports posting content to a particular URI on
|
||||
our site with data about the event. For these, you usually just need
|
||||
to add a new handler in `zerver/views/webhooks.py` (plus
|
||||
test/document/etc.). An example commit implementing a new webhook.
|
||||
test/document/etc.). An example commit implementing a new webhook is:
|
||||
https://github.com/zulip/zulip/pull/324.
|
||||
|
||||
2. Python script integrations (examples: SVN, Git), where we can get
|
||||
|
||||
218
docs/new-feature-tutorial.md
Normal file
218
docs/new-feature-tutorial.md
Normal file
@@ -0,0 +1,218 @@
|
||||
How to write a new application feature
|
||||
======================================
|
||||
|
||||
The changes needed to add a new feature will vary, of course, but this
|
||||
document provides a general outline of what you may need to do, as well
|
||||
as an example of the specific steps needed to add a new feature: adding
|
||||
a new option to the application that is dynamically synced through the
|
||||
data system in real-time to all browsers the user may have open.
|
||||
|
||||
General Process
|
||||
---------------
|
||||
|
||||
### Adding a field to the database
|
||||
|
||||
**Update the model:** The server accesses the underlying database in
|
||||
`zerver/ models.py`. Add a new field in the appropriate class.
|
||||
|
||||
**Create and run the migration:** To create and apply a migration, run:
|
||||
:
|
||||
|
||||
./manage.py makemigrations ./manage.py migrate
|
||||
|
||||
**Test your changes:** Once you've run the migration, restart memcached
|
||||
on your development server (`/etc/init.d/memcached restart`) and then
|
||||
restart `run-dev.py` to avoid interacting with cached objects.
|
||||
|
||||
### Backend changes
|
||||
|
||||
**Database interaction:** Add any necessary code for updating and
|
||||
interacting with the database in `zerver/lib/actions.py`. It should
|
||||
update the database and send an event announcing the change.
|
||||
|
||||
**Application state:** Modify the `fetch_initial_state_data` and
|
||||
`apply_events` functions in `zerver/lib/actions.py` to update the state
|
||||
based on the event you just created.
|
||||
|
||||
**Backend implementation:** Make any other modifications to the backend
|
||||
required for your change.
|
||||
|
||||
**Testing:** At the very least, add a test of your event data flowing
|
||||
through the system in `test_events.py`.
|
||||
|
||||
### Frontend changes
|
||||
|
||||
**JavaScript:** Zulip's JavaScript is located in the directory
|
||||
`static/js/`. The exact files you may need to change depend on your
|
||||
feature. If you've added a new event that is sent to clients, be sure to
|
||||
add a handler for it to `static/js/server_events.js`.
|
||||
|
||||
**CSS:** The primary CSS file is `static/styles/zulip.css`. If your new
|
||||
feature requires UI changes, you may need to add additional CSS to this
|
||||
file.
|
||||
|
||||
**Templates:** The initial page structure is rendered via Jinja2
|
||||
templates located in `templates/zerver`. For JavaScript, Zulip uses
|
||||
Handlebars templates located in `static/templates`. Templates are
|
||||
precompiled as part of the build/deploy process.
|
||||
|
||||
**Testing:** There are two types of frontend tests: node-based unit
|
||||
tests and blackbox end-to-end tests. The blackbox tests are run in a
|
||||
headless browser using Casper.js and are located in
|
||||
`frontend_tests/casper_tests/`. The unit tests use Node's `assert`
|
||||
module are located in `frontend_tests/node_tests/`. For more information
|
||||
on writing and running tests see the testing documentation \<testing\>.
|
||||
|
||||
Example Feature
|
||||
---------------
|
||||
|
||||
This example describes the process of adding a new setting to Zulip: a
|
||||
flag that restricts inviting new users to admins only (the default
|
||||
behavior is that any user can invite other users). It is based on an
|
||||
actual Zulip feature, and you can review [the original commit in the
|
||||
Zulip git
|
||||
repo](https://github.com/zulip/zulip/commit/5b7f3466baee565b8e5099bcbd3e1ccdbdb0a408).
|
||||
(Note that Zulip has since been upgraded from Django 1.6 to 1.8, so the
|
||||
migration format has changed.)
|
||||
|
||||
First, update the database and model to store the new setting. Add a new
|
||||
boolean field, `realm_invite_by_admins_only`, to the Realm model in
|
||||
`zerver/models.py`.
|
||||
|
||||
Then create a Django migration that adds a new field,
|
||||
`invite_by_admins_only`, to the `zerver_realm` table.
|
||||
|
||||
In `zerver/lib/actions.py`, create a new function named
|
||||
`do_set_realm_invite_by_admins_only`. This function will update the
|
||||
database and trigger an event to notify clients when this setting
|
||||
changes. In this case there was an exisiting `realm|update` event type
|
||||
which was used for setting similar flags on the Realm model, so it was
|
||||
possible to add a new property to that event rather than creating a new
|
||||
one. The property name matches the database field to make it easy to
|
||||
understand what it indicates.
|
||||
|
||||
The second argument to `send_event` is the list of users whose browser
|
||||
sessions should be notified. Depending on the setting, this can be a
|
||||
single user (if the setting is a personal one, like time display
|
||||
format), only members in a particular stream or all active users in a
|
||||
realm. :
|
||||
|
||||
# zerver/lib/actions.py
|
||||
|
||||
def do_set_realm_invite_by_admins_only(realm, invite_by_admins_only):
|
||||
realm.invite_by_admins_only = invite_by_admins_only
|
||||
realm.save(update_fields=['invite_by_admins_only'])
|
||||
event = dict(
|
||||
type="realm",
|
||||
op="update",
|
||||
property='invite_by_admins_only',
|
||||
value=invite_by_admins_only,
|
||||
)
|
||||
send_event(event, active_user_ids(realm))
|
||||
return {}
|
||||
|
||||
You then need to add code that will handle the event and update the
|
||||
application state. In `zerver/lib/actions.py` update the
|
||||
`fetch_initial_state` and `apply_events` functions. :
|
||||
|
||||
def fetch_initial_state_data(user_profile, event_types, queue_id):
|
||||
# ...
|
||||
state['realm_invite_by_admins_only'] = user_profile.realm.invite_by_admins_only`
|
||||
|
||||
In this case you don't need to change `apply_events` because there is
|
||||
already code that will correctly handle the realm update event type: :
|
||||
|
||||
def apply_events(state, events, user_profile):
|
||||
for event in events:
|
||||
# ...
|
||||
elif event['type'] == 'realm':
|
||||
field = 'realm_' + event['property']
|
||||
state[field] = event['value']
|
||||
|
||||
You then need to add a view for clients to access that will call the
|
||||
newly-added `actions.py` code to update the database. This example
|
||||
feature adds a new parameter that should be sent to clients when the
|
||||
application loads and be accessible via JavaScript, and there is already
|
||||
a view that does this for related flags: `update_realm`. So in this
|
||||
case, we can add out code to the exisiting view instead of creating a
|
||||
new one. :
|
||||
|
||||
# zerver/views/__init__.py
|
||||
|
||||
def home(request):
|
||||
# ...
|
||||
page_params = dict(
|
||||
# ...
|
||||
realm_invite_by_admins_only = register_ret['realm_invite_by_admins_only'],
|
||||
# ...
|
||||
)
|
||||
|
||||
Since this feature also adds a checkbox to the admin page, and adds a
|
||||
new property the Realm model that can be modified from there, you also
|
||||
need to make changes to the `update_realm` function in the same file: :
|
||||
|
||||
# zerver/views/__init__.py
|
||||
|
||||
def update_realm(request, user_profile,
|
||||
name=REQ(validator=check_string, default=None),
|
||||
restricted_to_domain=REQ(validator=check_bool, default=None),
|
||||
invite_by_admins_only=REQ(validator=check_bool,default=None)):
|
||||
|
||||
# ...
|
||||
|
||||
if invite_by_admins_only is not None and
|
||||
realm.invite_by_admins_only != invite_by_admins_only:
|
||||
do_set_realm_invite_by_admins_only(realm, invite_by_admins_only)
|
||||
data['invite_by_admins_only'] = invite_by_admins_only
|
||||
|
||||
Then make the required front end changes: in this case a checkbox needs
|
||||
to be added to the admin page (and its value added to the data sent back
|
||||
to server when a realm is updated) and the change event needs to be
|
||||
handled on the client.
|
||||
|
||||
To add the checkbox to the admin page, modify the relevant template,
|
||||
`static/templates/admin_tab.handlebars` (omitted here since it is
|
||||
relatively straightforward). Then add code to handle changes to the new
|
||||
form control in `static/js/admin.js`. :
|
||||
|
||||
var url = "/json/realm";
|
||||
var new_invite_by_admins_only =
|
||||
$("#id_realm_invite_by_admins_only").prop("checked");
|
||||
data[invite_by_admins_only] = JSON.stringify(new_invite_by_admins_only);
|
||||
|
||||
channel.patch({
|
||||
url: url,
|
||||
data: data,
|
||||
success: function (data) {
|
||||
# ...
|
||||
if (data.invite_by_admins_only) {
|
||||
ui.report_success("New users must be invited by an admin!", invite_by_admins_only_status);
|
||||
} else {
|
||||
ui.report_success("Any user may now invite new users!", invite_by_admins_only_status);
|
||||
}
|
||||
# ...
|
||||
}
|
||||
});
|
||||
|
||||
Finally, update `server_events.js` to handle related events coming from
|
||||
the server. :
|
||||
|
||||
# static/js/server_events.js
|
||||
|
||||
function get_events_success(events) {
|
||||
# ...
|
||||
var dispatch_event = function dispatch_event(event) {
|
||||
switch (event.type) {
|
||||
# ...
|
||||
case 'realm':
|
||||
if (event.op === 'update' && event.property === 'invite_by_admins_only') {
|
||||
page_params.realm_invite_by_admins_only = event.value;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Any code needed to update the UI should be placed in `dispatch_event`
|
||||
callback (rather than the `channel.patch`) function. This ensures the
|
||||
appropriate code will run even if the changes are made in another
|
||||
browser window. In this example most of the changes are on the backend,
|
||||
so no UI updates are required.
|
||||
@@ -1,216 +0,0 @@
|
||||
======================================
|
||||
How to write a new application feature
|
||||
======================================
|
||||
|
||||
The changes needed to add a new feature will vary, of course, but this document
|
||||
provides a general outline of what you may need to do, as well as an example of
|
||||
the specific steps needed to add a new feature: adding a new option to the
|
||||
application that is dynamically synced through the data system in real-time to
|
||||
all browsers the user may have open.
|
||||
|
||||
General Process
|
||||
===============
|
||||
|
||||
Adding a field to the database
|
||||
------------------------------
|
||||
|
||||
**Update the model:** The server accesses the underlying database in ``zerver/
|
||||
models.py``. Add a new field in the appropriate class.
|
||||
|
||||
**Create and run the migration:** To create and apply a migration, run: ::
|
||||
|
||||
./manage.py makemigrations
|
||||
./manage.py migrate
|
||||
|
||||
**Test your changes:** Once you've run the migration, restart memcached on your
|
||||
development server (``/etc/init.d/memcached restart``) and then restart
|
||||
``run-dev.py`` to avoid interacting with cached objects.
|
||||
|
||||
Backend changes
|
||||
---------------
|
||||
|
||||
**Database interaction:** Add any necessary code for updating and interacting
|
||||
with the database in ``zerver/lib/actions.py``. It should update the database and
|
||||
send an event announcing the change.
|
||||
|
||||
**Application state:** Modify the ``fetch_initial_state_data`` and ``apply_events``
|
||||
functions in ``zerver/lib/actions.py`` to update the state based on the event you
|
||||
just created.
|
||||
|
||||
**Backend implementation:** Make any other modifications to the backend required for
|
||||
your change.
|
||||
|
||||
**Testing:** At the very least, add a test of your event data flowing through
|
||||
the system in ``test_events.py``.
|
||||
|
||||
|
||||
Frontend changes
|
||||
----------------
|
||||
|
||||
**JavaScript:** Zulip's JavaScript is located in the directory ``static/js/``.
|
||||
The exact files you may need to change depend on your feature. If you've added a
|
||||
new event that is sent to clients, be sure to add a handler for it to
|
||||
``static/js/server_events.js``.
|
||||
|
||||
**CSS:** The primary CSS file is ``static/styles/zulip.css``. If your new
|
||||
feature requires UI changes, you may need to add additional CSS to this file.
|
||||
|
||||
**Templates:** The initial page structure is rendered via Jinja2 templates
|
||||
located in ``templates/zerver``. For JavaScript, Zulip uses Handlebars templates located in
|
||||
``static/templates``. Templates are precompiled as part of the build/deploy
|
||||
process.
|
||||
|
||||
**Testing:** There are two types of frontend tests: node-based unit tests and
|
||||
blackbox end-to-end tests. The blackbox tests are run in a headless browser
|
||||
using Casper.js and are located in ``frontend_tests/casper_tests/``. The unit
|
||||
tests use Node's ``assert`` module are located in ``frontend_tests/node_tests/``.
|
||||
For more information on writing and running tests see the :doc:`testing
|
||||
documentation <testing>`.
|
||||
|
||||
Example Feature
|
||||
===============
|
||||
|
||||
This example describes the process of adding a new setting to Zulip:
|
||||
a flag that restricts inviting new users to admins only (the default behavior
|
||||
is that any user can invite other users). It is based on an actual Zulip feature,
|
||||
and you can review `the original commit in the Zulip git repo <https://github.com/zulip/zulip/commit/5b7f3466baee565b8e5099bcbd3e1ccdbdb0a408>`_.
|
||||
(Note that Zulip has since been upgraded from Django 1.6 to 1.8, so the migration
|
||||
format has changed.)
|
||||
|
||||
First, update the database and model to store the new setting. Add a
|
||||
new boolean field, ``realm_invite_by_admins_only``, to the Realm model in
|
||||
``zerver/models.py``.
|
||||
|
||||
Then create a Django migration that adds a new field, ``invite_by_admins_only``,
|
||||
to the ``zerver_realm`` table.
|
||||
|
||||
In ``zerver/lib/actions.py``, create a new function named
|
||||
``do_set_realm_invite_by_admins_only``. This function will update the database
|
||||
and trigger an event to notify clients when this setting changes. In this case
|
||||
there was an exisiting ``realm|update`` event type which was used for setting
|
||||
similar flags on the Realm model, so it was possible to add a new property to
|
||||
that event rather than creating a new one. The property name matches the
|
||||
database field to make it easy to understand what it indicates.
|
||||
|
||||
The second argument to ``send_event`` is the list of users whose browser
|
||||
sessions should be notified. Depending on the setting, this can be a single user
|
||||
(if the setting is a personal one, like time display format), only members in a
|
||||
particular stream or all active users in a realm. ::
|
||||
|
||||
# zerver/lib/actions.py
|
||||
|
||||
def do_set_realm_invite_by_admins_only(realm, invite_by_admins_only):
|
||||
realm.invite_by_admins_only = invite_by_admins_only
|
||||
realm.save(update_fields=['invite_by_admins_only'])
|
||||
event = dict(
|
||||
type="realm",
|
||||
op="update",
|
||||
property='invite_by_admins_only',
|
||||
value=invite_by_admins_only,
|
||||
)
|
||||
send_event(event, active_user_ids(realm))
|
||||
return {}
|
||||
|
||||
You then need to add code that will handle the event and update the application
|
||||
state. In ``zerver/lib/actions.py`` update the ``fetch_initial_state`` and
|
||||
``apply_events`` functions. ::
|
||||
|
||||
def fetch_initial_state_data(user_profile, event_types, queue_id):
|
||||
# ...
|
||||
state['realm_invite_by_admins_only'] = user_profile.realm.invite_by_admins_only`
|
||||
|
||||
In this case you don't need to change ``apply_events`` because there is already
|
||||
code that will correctly handle the realm update event type: ::
|
||||
|
||||
def apply_events(state, events, user_profile):
|
||||
for event in events:
|
||||
# ...
|
||||
elif event['type'] == 'realm':
|
||||
field = 'realm_' + event['property']
|
||||
state[field] = event['value']
|
||||
|
||||
You then need to add a view for clients to access that will call the newly-added
|
||||
``actions.py`` code to update the database. This example feature adds a new
|
||||
parameter that should be sent to clients when the application loads and be
|
||||
accessible via JavaScript, and there is already a view that does this for
|
||||
related flags: ``update_realm``. So in this case, we can add out code to the
|
||||
exisiting view instead of creating a new one. ::
|
||||
|
||||
# zerver/views/__init__.py
|
||||
|
||||
def home(request):
|
||||
# ...
|
||||
page_params = dict(
|
||||
# ...
|
||||
realm_invite_by_admins_only = register_ret['realm_invite_by_admins_only'],
|
||||
# ...
|
||||
)
|
||||
|
||||
Since this feature also adds a checkbox to the admin page, and adds a new
|
||||
property the Realm model that can be modified from there, you also need to make
|
||||
changes to the ``update_realm`` function in the same file: ::
|
||||
|
||||
# zerver/views/__init__.py
|
||||
|
||||
def update_realm(request, user_profile,
|
||||
name=REQ(validator=check_string, default=None),
|
||||
restricted_to_domain=REQ(validator=check_bool, default=None),
|
||||
invite_by_admins_only=REQ(validator=check_bool,default=None)):
|
||||
|
||||
# ...
|
||||
|
||||
if invite_by_admins_only is not None and
|
||||
realm.invite_by_admins_only != invite_by_admins_only:
|
||||
do_set_realm_invite_by_admins_only(realm, invite_by_admins_only)
|
||||
data['invite_by_admins_only'] = invite_by_admins_only
|
||||
|
||||
Then make the required front end changes: in this case a checkbox needs to be
|
||||
added to the admin page (and its value added to the data sent back to server
|
||||
when a realm is updated) and the change event needs to be handled on the client.
|
||||
|
||||
To add the checkbox to the admin page, modify the relevant template,
|
||||
``static/templates/admin_tab.handlebars`` (omitted here since it is relatively
|
||||
straightforward). Then add code to handle changes to the new form control in
|
||||
``static/js/admin.js``. ::
|
||||
|
||||
var url = "/json/realm";
|
||||
var new_invite_by_admins_only =
|
||||
$("#id_realm_invite_by_admins_only").prop("checked");
|
||||
data[invite_by_admins_only] = JSON.stringify(new_invite_by_admins_only);
|
||||
|
||||
channel.patch({
|
||||
url: url,
|
||||
data: data,
|
||||
success: function (data) {
|
||||
# ...
|
||||
if (data.invite_by_admins_only) {
|
||||
ui.report_success("New users must be invited by an admin!", invite_by_admins_only_status);
|
||||
} else {
|
||||
ui.report_success("Any user may now invite new users!", invite_by_admins_only_status);
|
||||
}
|
||||
# ...
|
||||
}
|
||||
});
|
||||
|
||||
Finally, update ``server_events.js`` to handle related events coming from the
|
||||
server. ::
|
||||
|
||||
# static/js/server_events.js
|
||||
|
||||
function get_events_success(events) {
|
||||
# ...
|
||||
var dispatch_event = function dispatch_event(event) {
|
||||
switch (event.type) {
|
||||
# ...
|
||||
case 'realm':
|
||||
if (event.op === 'update' && event.property === 'invite_by_admins_only') {
|
||||
page_params.realm_invite_by_admins_only = event.value;
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Any code needed to update the UI should be placed in ``dispatch_event`` callback
|
||||
(rather than the ``channel.patch``) function. This ensures the appropriate code
|
||||
will run even if the changes are made in another browser window. In this example
|
||||
most of the changes are on the backend, so no UI updates are required.
|
||||
|
||||
328
docs/testing.md
Normal file
328
docs/testing.md
Normal file
@@ -0,0 +1,328 @@
|
||||
Testing and writing tests
|
||||
=========================
|
||||
|
||||
Running tests
|
||||
-------------
|
||||
|
||||
To run everything, just use `./tools/test-all`. This runs lint checks,
|
||||
web frontend / whole-system blackbox tests, and backend Django tests.
|
||||
|
||||
If you want to run individual parts, see the various commands inside
|
||||
that script.
|
||||
|
||||
### Schema and initial data changes
|
||||
|
||||
If you change the database schema or change the initial test data, you
|
||||
have to regenerate the pristine test database by running
|
||||
`tools/do-destroy-rebuild-test-database`.
|
||||
|
||||
### Wiping the test databases
|
||||
|
||||
You should first try running: `tools/do-destroy-rebuild-test-database`
|
||||
|
||||
If that fails you should try to do:
|
||||
|
||||
sudo -u postgres psql
|
||||
> DROP DATABASE zulip_test;
|
||||
> DROP DATABASE zulip_test_template;
|
||||
|
||||
and then run `tools/do-destroy-rebuild-test-database`
|
||||
|
||||
#### Recreating the postgres cluster
|
||||
|
||||
> **warning**
|
||||
>
|
||||
> **This is irreversible, so do it with care, and never do this anywhere
|
||||
> in production.**
|
||||
|
||||
If your postgres cluster (collection of databases) gets totally trashed
|
||||
permissions-wise, and you can't otherwise repair it, you can recreate
|
||||
it. On Ubuntu:
|
||||
|
||||
sudo pg_dropcluster --stop 9.1 main
|
||||
sudo pg_createcluster --locale=en_US.utf8 --start 9.1 main
|
||||
|
||||
### Backend Django tests
|
||||
|
||||
These live in `zerver/tests/tests.py` and `zerver/tests/test_*.py`. Run
|
||||
them with `tools/test-backend`.
|
||||
|
||||
### Web frontend black-box casperjs tests
|
||||
|
||||
These live in `frontend_tests/casper_tests/`. This is a "black box"
|
||||
test; we load the frontend in a real (headless) browser, from a real dev
|
||||
server, and simulate UI interactions like sending messages, narrowing,
|
||||
etc.
|
||||
|
||||
Since this is interacting with a real dev server, it can catch backend
|
||||
bugs as well.
|
||||
|
||||
You can run this with `./tools/test-js-with-casper` or as
|
||||
`./tools/test-js-with-casper 05-settings.js` to run a single test file
|
||||
from `frontend_tests/casper_tests/`.
|
||||
|
||||
#### Debugging Casper.JS
|
||||
|
||||
Casper.js (via PhantomJS) has support for remote debugging. However, it
|
||||
is not perfect. Here are some steps for using it and gotchas you might
|
||||
want to know.
|
||||
|
||||
To turn on remote debugging, pass `--remote-debug` to the
|
||||
`./frontend_tests/run-casper` script. This will run the tests with port
|
||||
`7777` open for remote debugging. You can now connect to
|
||||
`localhost:7777` in a Webkit browser. Somewhat recent versions of Chrome
|
||||
or Safari might be required.
|
||||
|
||||
- When connecting to the remote debugger, you will see a list of
|
||||
pages, probably 2. One page called `about:blank` is the headless
|
||||
page in which the CasperJS test itself is actually running in. This
|
||||
is where your test code is.
|
||||
- The other page, probably `localhost:9981`, is the Zulip page that
|
||||
the test is testing---that is, the page running our app that our
|
||||
test is exercising.
|
||||
|
||||
Since the tests are now running, you can open the `about:blank` page,
|
||||
switch to the Scripts tab, and open the running `0x-foo.js` test. If you
|
||||
set a breakpoint and it is hit, the inspector will pause and you can do
|
||||
your normal JS debugging. You can also put breakpoints in the Zulip
|
||||
webpage itself if you wish to inspect the state of the Zulip frontend.
|
||||
|
||||
You can also check the screenshots of failed tests at `/tmp/casper-failure*.png`.
|
||||
|
||||
If you need to use print debugging in casper, you can do using
|
||||
`casper.log`; see <http://docs.casperjs.org/en/latest/logging.html> for
|
||||
details.
|
||||
|
||||
An additional debugging technique is to enable verbose mode in the
|
||||
Casper tests; you can do this by adding to the top of the relevant test
|
||||
file the following:
|
||||
|
||||
> var casper = require('casper').create({
|
||||
> verbose: true,
|
||||
> logLevel: "debug"
|
||||
> });
|
||||
|
||||
This can sometimes give insight into exactly what's happening.
|
||||
|
||||
### Web frontend unit tests
|
||||
|
||||
As an alternative to the black-box whole-app testing, you can unit test
|
||||
individual JavaScript files that use the module pattern. For example, to
|
||||
test the `foobar.js` file, you would first add the following to the
|
||||
bottom of `foobar.js`:
|
||||
|
||||
> if (typeof module !== 'undefined') {
|
||||
> module.exports = foobar;
|
||||
> }
|
||||
|
||||
This makes `foobar.js` follow the CommonJS module pattern, so it can be
|
||||
required in Node.js, which runs our tests.
|
||||
|
||||
Now create `frontend_tests/node_tests/foobar.js`. At the top, require
|
||||
the [Node.js assert module](http://nodejs.org/api/assert.html), and the
|
||||
module you're testing, like so:
|
||||
|
||||
> var assert = require('assert');
|
||||
> var foobar = require('js/foobar.js');
|
||||
|
||||
(If the module you're testing depends on other modules, or modifies
|
||||
global state, you need to also read [the next
|
||||
section](handling-dependencies_).)
|
||||
|
||||
Define and call some tests using the [assert
|
||||
module](http://nodejs.org/api/assert.html). Note that for "equal"
|
||||
asserts, the *actual* value comes first, the *expected* value second.
|
||||
|
||||
> (function test_somefeature() {
|
||||
> assert.strictEqual(foobar.somefeature('baz'), 'quux');
|
||||
> assert.throws(foobar.somefeature('Invalid Input'));
|
||||
> }());
|
||||
|
||||
The test runner (index.js) automatically runs all .js files in the
|
||||
frontend\_tests/node directory.
|
||||
|
||||
#### Coverage reports
|
||||
|
||||
You can automatically generate coverage reports for the JavaScript unit
|
||||
tests. To do so, install istanbul:
|
||||
|
||||
> sudo npm install -g istanbul
|
||||
|
||||
And run test-js-with-node with the 'cover' parameter:
|
||||
|
||||
> tools/test-js-with-node cover
|
||||
|
||||
Then open `coverage/lcov-report/js/index.html` in your browser. Modules
|
||||
we don't test *at all* aren't listed in the report, so this tends to
|
||||
overstate how good our overall coverage is, but it's accurate for
|
||||
individual files. You can also click a filename to see the specific
|
||||
statements and branches not tested. 100% branch coverage isn't
|
||||
necessarily possible, but getting to at least 80% branch coverage is a
|
||||
good goal.
|
||||
|
||||
Writing tests
|
||||
-------------
|
||||
|
||||
### Writing Casper tests
|
||||
|
||||
Probably the easiest way to learn how to write Casper tests is to study
|
||||
some of the existing test files. There are a few tips that can be useful
|
||||
for writing Casper tests in addition to the debugging notes below:
|
||||
|
||||
- Run just the file containing your new tests as described above to
|
||||
have a fast debugging cycle.
|
||||
- With frontend tests in general, it's very important to write your
|
||||
code to wait for the right events. Before essentially every action
|
||||
you take on the page, you'll want to use `waitForSelector`,
|
||||
`waitUntilVisible`, or a similar function to make sure the page or
|
||||
elemant is ready before you interact with it. For instance, if you
|
||||
want to click a button that you can select via `#btn-submit`, and
|
||||
then check that it causes `success-elt` to appear, you'll want to
|
||||
write something like:
|
||||
|
||||
casper.waitForSelector("#btn-submit", function () {
|
||||
casper.click('#btn-submit')
|
||||
casper.test.assertExists("#success-elt");
|
||||
});
|
||||
|
||||
This will ensure that the element is present before the interaction
|
||||
is attempted. The various wait functions supported in Casper are
|
||||
documented in the Casper here:
|
||||
<http://docs.casperjs.org/en/latest/modules/casper.html#waitforselector>
|
||||
and the various assert statements available are documented here:
|
||||
<http://docs.casperjs.org/en/latest/modules/tester.html#the-tester-prototype>
|
||||
- Casper uses CSS3 selectors; you can often save time by testing and
|
||||
debugigng your selectors on the relevant page of the Zulip
|
||||
development app in the Chrome javascript console by using e.g.
|
||||
`$$("#settings-dropdown")`.
|
||||
- The test suite uses a smaller set of default user accounts and other
|
||||
data initialized in the database than the development environment;
|
||||
to see what differs check out the section related to
|
||||
`options["test_suite"]` in
|
||||
`zilencer/management/commands/populate_db.py`.
|
||||
- Casper effectively runs your test file in two phases -- first it
|
||||
runs the code in the test file, which for most test files will just
|
||||
collect a series of steps (each being a `casper.then` or
|
||||
`casper.wait...` call). Then, usually at the end of the test file,
|
||||
you'll have a `casper.run` call which actually runs that series of
|
||||
steps. This means that if you write code in your test file outside a
|
||||
`casper.then` or `casper.wait...` method, it will actually run
|
||||
before all the Casper test steps that are declared in the file,
|
||||
which can lead to confusing failures where the new code you write in
|
||||
between two `casper.then` blocks actually runs before either of
|
||||
them. See this for more details about how Casper works:
|
||||
<http://docs.casperjs.org/en/latest/faq.html#how-does-then-and-the-step-stack-work>
|
||||
|
||||
### Handling dependencies in unit tests
|
||||
|
||||
The following scheme helps avoid tests leaking globals between each
|
||||
other.
|
||||
|
||||
First, if you can avoid globals, do it, and the code that is directly
|
||||
under test can simply be handled like this:
|
||||
|
||||
> var search = require('js/search_suggestion.js');
|
||||
|
||||
For deeper dependencies, you want to categorize each module as follows:
|
||||
|
||||
- Exercise the module's real code for deeper, more realistic testing?
|
||||
- Stub out the module's interface for more control, speed, and
|
||||
isolation?
|
||||
- Do some combination of the above?
|
||||
|
||||
For all the modules where you want to run actual code, add a statement
|
||||
like the following to the top of your test file:
|
||||
|
||||
> add_dependencies({
|
||||
> _: 'third/underscore/underscore.js',
|
||||
> util: 'js/util.js',
|
||||
> Dict: 'js/dict.js',
|
||||
> Handlebars: 'handlebars',
|
||||
> Filter: 'js/filter.js',
|
||||
> typeahead_helper: 'js/typeahead_helper.js',
|
||||
> stream_data: 'js/stream_data.js',
|
||||
> narrow: 'js/narrow.js'
|
||||
> });
|
||||
|
||||
For modules that you want to completely stub out, please use a pattern
|
||||
like this:
|
||||
|
||||
> set_global('page_params', {
|
||||
> email: 'bob@zulip.com'
|
||||
> });
|
||||
>
|
||||
> // then maybe further down
|
||||
> global.page_params.email = 'alice@zulip.com';
|
||||
|
||||
Finally, there's the hybrid situation, where you want to borrow some of
|
||||
a module's real functionality but stub out other pieces. Obviously, this
|
||||
is a pretty strong smell that the other module might be lacking in
|
||||
cohesion, but that code might be outside your jurisdiction. The pattern
|
||||
here is this:
|
||||
|
||||
> // Use real versions of parse/unparse
|
||||
> var narrow = require('js/narrow.js');
|
||||
> set_global('narrow', {
|
||||
> parse: narrow.parse,
|
||||
> unparse: narrow.unparse
|
||||
> });
|
||||
>
|
||||
> // But later, I want to stub the stream without having to call super-expensive
|
||||
> // real code like narrow.activate().
|
||||
> global.narrow.stream = function () {
|
||||
> return 'office';
|
||||
> };
|
||||
|
||||
Manual testing (local app + web browser)
|
||||
----------------------------------------
|
||||
|
||||
### Clearing the manual testing database
|
||||
|
||||
You can use:
|
||||
|
||||
./tools/do-destroy-rebuild-database
|
||||
|
||||
to drop the database on your development environment and repopulate
|
||||
your it with the Shakespeare characters and some test messages between
|
||||
them. This is run automatically as part of the development
|
||||
environment setup process, but is occasionally useful when you want to
|
||||
return to a clean state for testing.
|
||||
|
||||
### JavaScript manual testing
|
||||
|
||||
debug.js has some tools for profiling Javascript code, including:
|
||||
|
||||
- \`print\_elapsed\_time\`: Wrap a function with it to print the time
|
||||
that function takes to the javascript console.
|
||||
- \`IterationProfiler\`: Profile part of looping constructs (like a
|
||||
for loop or \$.each). You mark sections of the iteration body and
|
||||
the IterationProfiler will sum the costs of those sections over all
|
||||
iterations.
|
||||
|
||||
Chrome has a very good debugger and inspector in its developer tools.
|
||||
Firebug for Firefox is also pretty good. They both have profilers, but
|
||||
Chrome's is a sampling profiler while Firebug's is an instrumenting
|
||||
profiler. Using them both can be helpful because they provide different
|
||||
information.
|
||||
|
||||
Python 3 Compatibility
|
||||
----------------------
|
||||
|
||||
Zulip is working on supporting Python 3, and all new code in Zulip
|
||||
should be Python 2+3 compatible. We have converted most of the codebase
|
||||
to be compatible with Python 3 using a suite of 2to3 conversion tools
|
||||
and some manual work. In order to avoid regressions in that
|
||||
compatibility as we continue to develop new features in zulip, we have a
|
||||
special tool, tools/check-py3, which checks all code for Python 3
|
||||
syntactic compatibility by running a subset of the automated migration
|
||||
tools and checking if they trigger any changes. tools/check-py3 is run
|
||||
automatically in Zulip's Travis CI tests to avoid any regressions, but
|
||||
is not included in test-all since it is quite slow.
|
||||
|
||||
To run tooks/check-py3, you need to install the modernize and future
|
||||
python packages (which are in the development environment's
|
||||
requirements.txt file).
|
||||
|
||||
To run check-py3 on just the python files in a particular directory, you
|
||||
can change the current working directory (e.g. cd zerver/) and run
|
||||
check-py3 from there.
|
||||
380
docs/testing.rst
380
docs/testing.rst
@@ -1,380 +0,0 @@
|
||||
=========================
|
||||
Testing and writing tests
|
||||
=========================
|
||||
|
||||
Running tests
|
||||
=============
|
||||
|
||||
To run everything, just use ``./tools/test-all``. This runs lint checks,
|
||||
web frontend / whole-system blackbox tests, and backend Django tests.
|
||||
|
||||
If you want to run individual parts, see the various commands inside
|
||||
that script.
|
||||
|
||||
Schema and initial data changes
|
||||
-------------------------------
|
||||
|
||||
If you change the database schema or change the initial test data, you
|
||||
have to regenerate the pristine test database by running
|
||||
``tools/do-destroy-rebuild-test-database``.
|
||||
|
||||
Wiping the test databases
|
||||
-------------------------
|
||||
|
||||
You should first try running: ``tools/do-destroy-rebuild-test-database``
|
||||
|
||||
If that fails you should try to do:
|
||||
|
||||
::
|
||||
|
||||
sudo -u postgres psql
|
||||
> DROP DATABASE zulip_test;
|
||||
> DROP DATABASE zulip_test_template;
|
||||
|
||||
and then run ``tools/do-destroy-rebuild-test-database``
|
||||
|
||||
Recreating the postgres cluster
|
||||
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
||||
|
||||
.. warning::
|
||||
|
||||
**This is irreversible, so do it with care, and never do this anywhere
|
||||
in production.**
|
||||
|
||||
If your postgres cluster (collection of databases) gets totally trashed
|
||||
permissions-wise, and you can't otherwise repair it, you can recreate
|
||||
it. On Ubuntu:
|
||||
|
||||
::
|
||||
|
||||
sudo pg_dropcluster --stop 9.1 main
|
||||
sudo pg_createcluster --locale=en_US.utf8 --start 9.1 main
|
||||
|
||||
Backend Django tests
|
||||
--------------------
|
||||
|
||||
These live in ``zerver/tests/tests.py`` and
|
||||
``zerver/tests/test_*.py``. Run them with ``tools/test-backend``.
|
||||
|
||||
Web frontend black-box casperjs tests
|
||||
-------------------------------------
|
||||
|
||||
These live in ``frontend_tests/casper_tests/``. This is a "black box"
|
||||
test; we load the frontend in a real (headless) browser, from a real dev
|
||||
server, and simulate UI interactions like sending messages, narrowing,
|
||||
etc.
|
||||
|
||||
Since this is interacting with a real dev server, it can catch backend
|
||||
bugs as well.
|
||||
|
||||
You can run this with ``./tools/test-js-with-casper`` or as
|
||||
``./tools/test-js-with-casper 05-settings.js`` to run a single test
|
||||
file from ``frontend_tests/casper_tests/``.
|
||||
|
||||
|
||||
Debugging Casper.JS
|
||||
~~~~~~~~~~~~~~~~~~~
|
||||
|
||||
Casper.js (via PhantomJS) has support for remote debugging. However, it
|
||||
is not perfect. Here are some steps for using it and gotchas you might
|
||||
want to know.
|
||||
|
||||
To turn on remote debugging, pass ``--remote-debug`` to the
|
||||
``./frontend_tests/run-casper`` script. This will run the tests with
|
||||
port ``7777`` open for remote debugging. You can now connect to
|
||||
``localhost:7777`` in a Webkit browser. Somewhat recent versions of
|
||||
Chrome or Safari might be required.
|
||||
|
||||
- When connecting to the remote debugger, you will see a list of pages,
|
||||
probably 2. One page called ``about:blank`` is the headless page in
|
||||
which the CasperJS test itself is actually running in. This is where
|
||||
your test code is.
|
||||
- The other page, probably ``localhost:9981``, is the Zulip page that
|
||||
the test is testing---that is, the page running our app that our test
|
||||
is exercising.
|
||||
|
||||
Since the tests are now running, you can open the ``about:blank`` page,
|
||||
switch to the Scripts tab, and open the running ``0x-foo.js`` test. If
|
||||
you set a breakpoint and it is hit, the inspector will pause and you can
|
||||
do your normal JS debugging. You can also put breakpoints in the Zulip
|
||||
webpage itself if you wish to inspect the state of the Zulip frontend.
|
||||
|
||||
You can also check the screenshots of failed tests at ``/tmp/casper-failure*.png``.
|
||||
|
||||
If you need to use print debugging in casper, you can do using
|
||||
``casper.log``; see http://docs.casperjs.org/en/latest/logging.html
|
||||
for details.
|
||||
|
||||
An additional debugging technique is to enable verbose mode in the
|
||||
Casper tests; you can do this by adding to the top of the relevant
|
||||
test file the following:
|
||||
|
||||
::
|
||||
|
||||
var casper = require('casper').create({
|
||||
verbose: true,
|
||||
logLevel: "debug"
|
||||
});
|
||||
|
||||
This can sometimes give insight into exactly what's happening.
|
||||
|
||||
Web frontend unit tests
|
||||
-----------------------
|
||||
|
||||
As an alternative to the black-box whole-app testing, you can unit test
|
||||
individual JavaScript files that use the module pattern. For example, to
|
||||
test the ``foobar.js`` file, you would first add the following to the
|
||||
bottom of ``foobar.js``:
|
||||
|
||||
::
|
||||
|
||||
if (typeof module !== 'undefined') {
|
||||
module.exports = foobar;
|
||||
}
|
||||
|
||||
This makes ``foobar.js`` follow the CommonJS module pattern, so it can
|
||||
be required in Node.js, which runs our tests.
|
||||
|
||||
Now create ``frontend_tests/node_tests/foobar.js``. At the top, require
|
||||
the `Node.js assert module <http://nodejs.org/api/assert.html>`__, and
|
||||
the module you're testing, like so:
|
||||
|
||||
::
|
||||
|
||||
var assert = require('assert');
|
||||
var foobar = require('js/foobar.js');
|
||||
|
||||
(If the module you're testing depends on other modules, or modifies
|
||||
global state, you need to also read `the next section`__.)
|
||||
|
||||
__ handling-dependencies_
|
||||
|
||||
Define and call some tests using the `assert
|
||||
module <http://nodejs.org/api/assert.html>`__. Note that for "equal"
|
||||
asserts, the *actual* value comes first, the *expected* value second.
|
||||
|
||||
::
|
||||
|
||||
(function test_somefeature() {
|
||||
assert.strictEqual(foobar.somefeature('baz'), 'quux');
|
||||
assert.throws(foobar.somefeature('Invalid Input'));
|
||||
}());
|
||||
|
||||
The test runner (index.js) automatically runs all .js files in the
|
||||
frontend_tests/node directory.
|
||||
|
||||
.. _handling-dependencies:
|
||||
|
||||
Coverage reports
|
||||
~~~~~~~~~~~~~~~~
|
||||
|
||||
You can automatically generate coverage reports for the JavaScript unit
|
||||
tests. To do so, install istanbul:
|
||||
|
||||
::
|
||||
|
||||
sudo npm install -g istanbul
|
||||
|
||||
And run test-js-with-node with the 'cover' parameter:
|
||||
|
||||
::
|
||||
|
||||
tools/test-js-with-node cover
|
||||
|
||||
Then open ``coverage/lcov-report/js/index.html`` in your browser.
|
||||
Modules we don't test *at all* aren't listed in the report, so this
|
||||
tends to overstate how good our overall coverage is, but it's accurate
|
||||
for individual files. You can also click a filename to see the specific
|
||||
statements and branches not tested. 100% branch coverage isn't
|
||||
necessarily possible, but getting to at least 80% branch coverage is a
|
||||
good goal.
|
||||
|
||||
|
||||
|
||||
Writing tests
|
||||
=============
|
||||
|
||||
|
||||
Writing Casper tests
|
||||
--------------------
|
||||
|
||||
Probably the easiest way to learn how to write Casper tests is to
|
||||
study some of the existing test files. There are a few tips that can
|
||||
be useful for writing Casper tests in addition to the debugging notes
|
||||
below:
|
||||
|
||||
- Run just the file containing your new tests as described above to
|
||||
have a fast debugging cycle.
|
||||
- With frontend tests in general, it's very important to write your
|
||||
code to wait for the right events. Before essentially every action
|
||||
you take on the page, you'll want to use ``waitForSelector``,
|
||||
``waitUntilVisible``, or a similar function to make sure the page or
|
||||
elemant is ready before you interact with it. For instance, if you
|
||||
want to click a button that you can select via ``#btn-submit``, and
|
||||
then check that it causes ``success-elt`` to appear, you'll want to
|
||||
write something like:
|
||||
|
||||
::
|
||||
|
||||
casper.waitForSelector("#btn-submit", function () {
|
||||
casper.click('#btn-submit')
|
||||
casper.test.assertExists("#success-elt");
|
||||
});
|
||||
|
||||
This will ensure that the element is present before the interaction
|
||||
is attempted. The various wait functions supported in Casper are
|
||||
documented in the Casper here:
|
||||
http://docs.casperjs.org/en/latest/modules/casper.html#waitforselector
|
||||
and the various assert statements available are documented here:
|
||||
http://docs.casperjs.org/en/latest/modules/tester.html#the-tester-prototype
|
||||
- Casper uses CSS3 selectors; you can often save time by testing and
|
||||
debugigng your selectors on the relevant page of the Zulip
|
||||
development app in the Chrome javascript console by using
|
||||
e.g. ``$$("#settings-dropdown")``.
|
||||
- The test suite uses a smaller set of default user accounts and other
|
||||
data initialized in the database than the development environment;
|
||||
to see what differs check out the section related to
|
||||
``options["test_suite"]`` in
|
||||
``zilencer/management/commands/populate_db.py``.
|
||||
- Casper effectively runs your test file in two phases -- first it
|
||||
runs the code in the test file, which for most test files will just
|
||||
collect a series of steps (each being a ``casper.then`` or
|
||||
``casper.wait...`` call). Then, usually at the end of the test
|
||||
file, you'll have a ``casper.run`` call which actually runs that
|
||||
series of steps. This means that if you write code in your
|
||||
test file outside a ``casper.then`` or ``casper.wait...`` method, it
|
||||
will actually run before all the Casper test steps that are declared
|
||||
in the file, which can lead to confusing failures where the new code
|
||||
you write in between two ``casper.then`` blocks actually runs before
|
||||
either of them. See this for more details about how Casper works:
|
||||
http://docs.casperjs.org/en/latest/faq.html#how-does-then-and-the-step-stack-work
|
||||
|
||||
|
||||
Handling dependencies in unit tests
|
||||
-----------------------------------
|
||||
|
||||
The following scheme helps avoid tests leaking globals between each
|
||||
other.
|
||||
|
||||
First, if you can avoid globals, do it, and the code that is directly
|
||||
under test can simply be handled like this:
|
||||
|
||||
::
|
||||
|
||||
var search = require('js/search_suggestion.js');
|
||||
|
||||
For deeper dependencies, you want to categorize each module as follows:
|
||||
|
||||
- Exercise the module's real code for deeper, more realistic testing?
|
||||
- Stub out the module's interface for more control, speed, and
|
||||
isolation?
|
||||
- Do some combination of the above?
|
||||
|
||||
For all the modules where you want to run actual code, add a statement
|
||||
like the following to the top of your test file:
|
||||
|
||||
::
|
||||
|
||||
add_dependencies({
|
||||
_: 'third/underscore/underscore.js',
|
||||
util: 'js/util.js',
|
||||
Dict: 'js/dict.js',
|
||||
Handlebars: 'handlebars',
|
||||
Filter: 'js/filter.js',
|
||||
typeahead_helper: 'js/typeahead_helper.js',
|
||||
stream_data: 'js/stream_data.js',
|
||||
narrow: 'js/narrow.js'
|
||||
});
|
||||
|
||||
For modules that you want to completely stub out, please use a pattern
|
||||
like this:
|
||||
|
||||
::
|
||||
|
||||
set_global('page_params', {
|
||||
email: 'bob@zulip.com'
|
||||
});
|
||||
|
||||
// then maybe further down
|
||||
global.page_params.email = 'alice@zulip.com';
|
||||
|
||||
Finally, there's the hybrid situation, where you want to borrow some of
|
||||
a module's real functionality but stub out other pieces. Obviously, this
|
||||
is a pretty strong smell that the other module might be lacking in
|
||||
cohesion, but that code might be outside your jurisdiction. The pattern
|
||||
here is this:
|
||||
|
||||
::
|
||||
|
||||
// Use real versions of parse/unparse
|
||||
var narrow = require('js/narrow.js');
|
||||
set_global('narrow', {
|
||||
parse: narrow.parse,
|
||||
unparse: narrow.unparse
|
||||
});
|
||||
|
||||
// But later, I want to stub the stream without having to call super-expensive
|
||||
// real code like narrow.activate().
|
||||
global.narrow.stream = function () {
|
||||
return 'office';
|
||||
};
|
||||
|
||||
|
||||
Manual testing (local app + web browser)
|
||||
========================================
|
||||
|
||||
Clearing the manual testing database
|
||||
------------------------------------
|
||||
|
||||
You can use:
|
||||
|
||||
::
|
||||
|
||||
./tools/do-destroy-rebuild-database
|
||||
|
||||
to drop the database on your development environment and repopulate
|
||||
your it with the Shakespeare characters and some test messages between
|
||||
them. This is run automatically as part of the development
|
||||
environment setup process, but is occasionally useful when you want to
|
||||
return to a clean state for testing.
|
||||
|
||||
JavaScript manual testing
|
||||
-------------------------
|
||||
|
||||
`debug.js` has some tools for profiling Javascript code, including:
|
||||
|
||||
- `print_elapsed_time`: Wrap a function with it to print the time that
|
||||
function takes to the javascript console.
|
||||
- `IterationProfiler`: Profile part of looping constructs (like a for
|
||||
loop or $.each). You mark sections of the iteration body and the
|
||||
IterationProfiler will sum the costs of those sections over all
|
||||
iterations.
|
||||
|
||||
Chrome has a very good debugger and inspector in its developer tools.
|
||||
Firebug for Firefox is also pretty good. They both have profilers, but
|
||||
Chrome's is a sampling profiler while Firebug's is an instrumenting
|
||||
profiler. Using them both can be helpful because they provide
|
||||
different information.
|
||||
|
||||
Python 3 Compatibility
|
||||
======================
|
||||
|
||||
Zulip is working on supporting Python 3, and all new code in Zulip
|
||||
should be Python 2+3 compatible. We have converted most of the
|
||||
codebase to be compatible with Python 3 using a suite of 2to3
|
||||
conversion tools and some manual work. In order to avoid regressions
|
||||
in that compatibility as we continue to develop new features in zulip,
|
||||
we have a special tool, `tools/check-py3`, which checks all code for
|
||||
Python 3 syntactic compatibility by running a subset of the automated
|
||||
migration tools and checking if they trigger any changes.
|
||||
`tools/check-py3` is run automatically in Zulip's Travis CI tests to
|
||||
avoid any regressions, but is not included in `test-all` since it is
|
||||
quite slow.
|
||||
|
||||
To run `tooks/check-py3`, you need to install the `modernize` and
|
||||
`future` python packages (which are in the development environment's
|
||||
`requirements.txt` file).
|
||||
|
||||
To run `check-py3` on just the python files in a particular directory,
|
||||
you can change the current working directory (e.g. `cd zerver/`) and
|
||||
run `check-py3` from there.
|
||||
Reference in New Issue
Block a user