JSON.parse behaves as we want for numbers but for strings, we would
throw an error like 'unexpected token at position 0'. This meant we
couldn't read back the value set by `$input.data('val', 'text')`.
We change the user facing interface to allow specifying expected
number of error messages (default=1). Now an average test can look
like:
```
// We expect 3 error messages;
blueslip.expect('error', 'an error message', 3);
throwError();
throwError();
throwError();
blueslip.reset();
```
We now use `assert.throws()` to test that we're
properly calling `blueslip.fatal`.
In order to not break line coverage here, we have
to remove an unreachable `return` in `stream_data.js`.
Usually we test `fatal` for line coverage reasons.
Most places where we use `blueslip.fatal` fall in
these categories:
* the code is theoretically unreachable, but
we have `blueslip.fatal` for defensive reasons
* we have some upstream bug that we should just
fix
* the code should recover gracefully and just
use blueslip.errors()
It's possible that we should eliminate `blueslip.fatal`
from our API and just throw errors when really important
invariants get broken. This will make it more obvious
to somebody reading the code that we're not going to
continue after the call, and `blueslip` already knows
how to catch exceptions and report them.
Explicitly stubbing i18n in 48 different files
is mostly busy work at this point, and it doesn't
provide much signal, since often it's invoked
only to satisfy transitive dependencies.
Let's say you have module hello.js like so:
// hello.js
const hello_world = i18n.t('Hello world');
exports.get_greeting = () => hello_world;
And then two modules like this:
// apple.js
const hello = require('hello');
exports.foo = () => {
show_greeting(hello.get_greeting());
};
// banana.js
const hello = require('hello');
exports.foo = () => {
display_greeting(hello.get_greeting());
};
The test for apple.js could look like this,
and it won't crash due to the stub:
set_global('i18n', {t: () => {}});
zrequire('hello');
zrequire('apple');
Now let's say your write this broken version
of a test for banana.js:
zrequire('hello');
zrequire('banana');
If you run `./tools/test-js-with-node`, the
"banana" test will pass, because while it
does require "hello", it won't actually
*execute* the code that happens at require
time for "hello", because it's already in
the cache. Here is the code that gets
skipped:
const hello_world = i18n.t('Hello world');
But then if you try to run the banana test
individually, the above line of code will
cause the test to crash. And it will crash
even before you actually try to test the
meaningful code here:
exports.foo = () => {
display_greeting(hello.get_greeting());
};
This commit fixes this leak scenario by just
aggressively clearing out things from the
require cache.
This slows tests down by about 10%, which I think
is worth the extra safety here.
This is not always a behavior-preserving translation: _.extend mutates
its first argument. However, the code does not always appear to have
been written to expect that.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This is not always a behavior-preserving translation: $.extend mutates
its first argument. However, the code does not always appear to have
been written to expect that.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This allows us to collect coverage for Handlebars templates, and also
improves the readability of Handlebars-related stack traces.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This effectively reverts the following
commit from May 2019:
be527905ca
The implementation of closest() was a bit
buggy and complex. It's easy enough
to just stub the method yourself. We may
want to eventually re-implement it, but we
should follow the template of parent/set_parent.
If you fail to stub `closest` zjquery gives
a fairly helpful error message:
Error: You must create a stub for $("link-stub").closest
We now require all of our unit tests to handle
blueslip errors for warn/error/fatal. This
simplifies the zblueslip code to not have any
options passed in.
Most of the places changed here fell into two
categories:
- We were just missing a random piece of
setup data in a happy path test.
- We were testing error handling in just
a lazy way to ensure 100% coverage. Often
these error codepaths were fairly
contrived.
The one place where we especially lazy was
the stream_data tests, and those are now
more thorough.
This is relatively unobtrusive, and we don't send
anything to the server.
But any user can now enter blueslip.timings in the
console to see a map of how long things take in
milliseconds. We only record one timing per
event label (i.e. the most recent).
It's pretty easy to test this by just clicking
around. For 300 users/streams most things are
fast except for:
- initialize_everything
- manage streams (render_subscriptions)
Both do lots of nontrivial work, although
"manage streams" is a bit surprising, since
we're only measuring how long to build the
HTML from the templates (whereas the real
time is probably browser rendering costs).
We now require the actual tests to explicitly
to zrequire Dict, rather than magically adding this.
In one case, the use of Dict was clearly just for
the test (not the app), so I converted that an ordinary
JS object (see timerender.js).
ES6 and TS modules don’t insert themselves into `window`, so our tests
shouldn’t insert them either. Since the test `window` behaves like
`global` now, we can rely on legacy modules that do insert themselves
to do it themselves.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
This commit was originally automatically generated using `tools/lint
--only=eslint --fix`. It was then modified by tabbott to contain only
changes to a set of files that are unlikely to result in significant
merge conflicts with any open pull request, excluding about 20 files.
His plan is to merge the remaining changes with more precise care,
potentially involving merging parts of conflicting pull requests
before running the `eslint --fix` operation.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
ESLint won’t convert these automatically because it can’t rule out a
behavior difference arising from an access to a self-referential var
before it’s initialized:
> var x = (f => f())(() => x);
undefined
> let y = (f => f())(() => y);
Thrown:
ReferenceError: Cannot access 'y' before initialization
at repl:1:26
at repl:1:15
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
Because of the separate declarations, ESLint would convert them to
`let` and then trigger the `prefer-const` error.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
With webpack, variables declared in each file are already file-local
(Global variables need to be explicitly exported), so these IIFEs are
no longer needed.
Signed-off-by: Anders Kaseorg <andersk@mit.edu>
This adds the general machinery required, and sets it up for the file
`typing_status.js` as a first use case.
Co-authored-by: Anders Kaseorg <anders@zulipchat.com>
Rohitt Vashishtha
Steve Howell
Stefan Weil
Anders Kaseorg
Greg Price
Tim Abbott