diff --git a/docs/code-style.md b/docs/code-style.md new file mode 100644 index 00000000000000..818cee609c5037 --- /dev/null +++ b/docs/code-style.md @@ -0,0 +1,481 @@ +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 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 = $('').attr('id', zephyr.id); + +Passing a HTML string to jQuery is fine for simple hardcoded things: + + foo.append('foo

'); + +but avoid programmatically building complicated strings. + +We used to favor attaching behaviors in templates like so: + +

+ +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 python2.7` and not + `#!/usr/bin/env python2.7`. See commit `437d4aee` for an explanation + of why. Don't put such a 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. diff --git a/docs/code-style.rst b/docs/code-style.rst deleted file mode 100644 index 4ed8d253aeb2ea..00000000000000 --- a/docs/code-style.rst +++ /dev/null @@ -1,522 +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 `__) - - ``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 `__) -- 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 `__. - -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] `__, -`[2] `__, -`[3] `__ - -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 `__. 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 = $('').attr('id', zephyr.id); - -Passing a HTML string to jQuery is fine for simple hardcoded things:: - - foo.append('foo

'); - -but avoid programmatically building complicated strings. - -We used to favor attaching behaviors in templates like so:: - -

- -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 python2.7`` and not - ``#!/usr/bin/env python2.7``. See commit ``437d4aee`` for an explanation of - why. Don't put such a 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 `__ -- 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 - `__ - 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. diff --git a/docs/directory-structure.md b/docs/directory-structure.md new file mode 100644 index 00000000000000..3cd8be54261f37 --- /dev/null +++ b/docs/directory-structure.md @@ -0,0 +1,114 @@ +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`). diff --git a/docs/directory-structure.rst b/docs/directory-structure.rst deleted file mode 100644 index aa2132002e3154..00000000000000 --- a/docs/directory-structure.rst +++ /dev/null @@ -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). diff --git a/docs/front-end-build-process.md b/docs/front-end-build-process.md new file mode 100644 index 00000000000000..670bae7ce765b9 --- /dev/null +++ b/docs/front-end-build-process.md @@ -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 \ 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`. diff --git a/docs/front-end-build-process.rst b/docs/front-end-build-process.rst deleted file mode 100644 index e6e91efbc6edb5..00000000000000 --- a/docs/front-end-build-process.rst +++ /dev/null @@ -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 `_, 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 ` for -more information on the syntax.) - -Benefits of using CommonJS modules over the `IIFE -`_ -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``. - diff --git a/docs/new-feature-tutorial.md b/docs/new-feature-tutorial.md new file mode 100644 index 00000000000000..5f03354a24dcae --- /dev/null +++ b/docs/new-feature-tutorial.md @@ -0,0 +1,219 @@ +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 \. + +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. diff --git a/docs/new-feature-tutorial.rst b/docs/new-feature-tutorial.rst deleted file mode 100644 index 6f89d763223c60..00000000000000 --- a/docs/new-feature-tutorial.rst +++ /dev/null @@ -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 `. - -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 `_. -(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. - diff --git a/docs/testing.md b/docs/testing.md new file mode 100644 index 00000000000000..3027228ca0e824 --- /dev/null +++ b/docs/testing.md @@ -0,0 +1,324 @@ +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/tests/run` 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. + +If you need to use print debugging in casper, you can do using +`casper.log`; see 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: + + and the various assert statements available are documented here: + +- 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: + + +### 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) +---------------------------------------- + +### Setting up the manual testing database + + ./tools/do-destroy-rebuild-database + +Will populate your local database with all the usual accounts plus some +test messages involving Shakespeare characters. + +(This is run automatically as part of the development environment setup +process.) + +### 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. diff --git a/docs/testing.rst b/docs/testing.rst deleted file mode 100644 index 0b02d83d3ac4c4..00000000000000 --- a/docs/testing.rst +++ /dev/null @@ -1,376 +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/tests/run`` 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. - -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 `__, 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 `__. 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) -======================================== - -Setting up the manual testing database --------------------------------------- - -:: - - ./tools/do-destroy-rebuild-database - -Will populate your local database with all the usual accounts plus some -test messages involving Shakespeare characters. - -(This is run automatically as part of the development environment setup -process.) - -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.