Instead of prohibiting ‘return undefined’ (#8669), we require that a
function must return an explicit value always or never. This prevents
you from forgetting to return a value in some cases. It will also be
important for TypeScript, which distinguishes between undefined and
void.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
The code to run single files was added
in c15695e514,
and it's just kinda strange code.
We already do a lot of file logic in Python
to check for line-coverage, so it's easier
to just have all the logic in Python.
This adds a new feature--you can now specify
the actual file:
./tools/test-js-with-node frontend_tests/node_tests/people.js
(This is helpful if you just want to use
shell autocomplete.)
Another minor change is that if you specify
individual files, we won't sort them. This is
important when you're trying to hunt down test
leaks.
Finally, we have a nicer message if we can't find
the file.
There is good reason to do this (explanation is bit long!). With the
TypeScript migration, and the require and ES6 migrations that come
with it, we use require instead of set_global which loads the entire
module. Suppose we have a util module, which is used by some other
module, say message_store, and util is being required in message_store
since it is removed from window. Then, if a test zrequires
message_store first, and then zrequires the util module qand mocks one
of its methods, it will not be mocked for the message_store
module. The reason is:
1. zrequire('message_store') leads to require('util').
2. zrequire('util') removes the util module from cache and it is
reloaded. Now the util module in message_store and the one in
the test will be different and any updates to it in tests won't
be reflected in the actual code.
Which can lead to confusion for folks writing tests. I'll mention this
can be avoided doing zrequire('util') first but...that is not ideal.
And, since there was one outlier test that relied on this behavior,
we add the namespace.reset_module function.
ES and TypeScript modules are strict by default and don’t need this
directive. ESLint will remind us to add it to new CommonJS files and
remove it from ES and TypeScript modules.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This just lets us temporarily assign a value
to a field.
Differences with the "override" scheme:
* override only works on globals
* override (when passed in via run_test) will
just automatically clean up at the end of
the function
We want to undo overrides in reverse order,
which is important if you override the
same name more than once in the same
function.
Until today the code basically prevented
us from ever using the original implementation
of a name we stubbed, and most of them start
as undefined due to their parent modules
starting with `set_global`.
But I do want this proper, and I introduced
a tiny pitfall today.
There was only one place where we weren't
overriding a function, and the use case there
was fairly unique.
Knowing that we're dealing with only functions
will simplify override and allow us to add
features like detecting spurious stubs.
This forces us to more explicitly document at the
top of the file what dependencies we are stubbing,
plus it's less magical.
Also, we may want to do occasional audits of
set_global to clean up places where we mock
things like stream_data, which are probably just
easier to use the real version of now that we
have cleaner APIs to set up stream data.
The modules most affected by this change are our
dispatch-oriented tests--basically, all the
modules that test handling of Zulip events
plus hotkey.js.
Before we were making it impossible to reuse
the function again (so we were preventing
leaks), but it's fine to just restore the
original function, especially now that some
of our tests have grown bigger.
Prettier would do this anyway, but it’s separated out for a more
reviewable diff. Generated by ESLint.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Prettier would do this anyway, but it’s separated out for a more
reviewable diff. Generated by ESLint.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
Prettier would do this anyway, but it’s separated out for a more
reviewable diff. Generated by ESLint.
Signed-off-by: Anders Kaseorg <anders@zulip.com>
This adds support for a "spoiler" syntax in Zulip's markdown, which
can be used to hide content that one doesn't want to be immediately
visible without a click.
We use our own spoiler block syntax inspired by Zulip's existing quote
and math block markdown extensions, rather than requiring a token on
every line, as is present in some other markdown spoiler
implementations.
Fixes#5802.
Co-authored-by: Dylan Nugent <dylnuge@gmail.com>
We are trying to phase out the trigger-event way
of telling modules to do something.
In this case we not only remove the indirection
of the event handler, but we also get to remove
`compose_fade` from the `ui_init` startup sequence.
This also has us update `compose_fade` outside
the loop, although that's only a theoretical
improvement, since I don't think `peer_add` events
every actually include multiple streams.
To make the dispatch tests a little flatter, I
added a one-line change to zjsunit to add
`make_stub` to `global`.
To manually test:
* have Aaron reply to Denmark (keep compose box open)
* have Iago add Hamlet to Denmark
* have Hamlet unsubscribe
* Add action to mute topics.
* We don't need to store muted data per topic as previously planned.
* Moved launch topic test to the top so that they run on non-modified
data.
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>
Adds a electron_bridge event that takes in message id and reply recived from
the notification reply and sends a message. We do this in webapp so desktop
doesn't have to depend on narrow and channel modules.
We also modify zjunit to reset window.electron_bridge after every run
to avoid leaking it.
Not all our errors actually happen in the contexts we were
wrapping (e.g. `setTimeout` and `_.throttle`). Also this fixes the
neat Firefox inspector feature that shows you where your event
handlers for a given DOM element actually live.
Using this "semi-modern" browser event means that Safari 9 and older
and IE10 and older may not have our browser error reporting active;
that seems fine giving the vanishing market share of those browsers.
https://blog.sentry.io/2016/01/04/client-javascript-reporting-window-onerror
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
It seems like the de facto standard ES polyfill library these days,
and we already depend on it through simplebar.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
The minimal syntactic sugar it might provide isn’t worth the
unexpected side effects (including side effects on third party
modules).
For now, we allow zrequire to emulate the previous syntax in the Node
test suite, even though stealing part of the NPM namespace is
confusing.
Signed-off-by: Anders Kaseorg <anders@zulipchat.com>
We now use a Proxy to wrap zjquery elements, so
that we can detect callers trying to invoke methods
(or access attributes) that do not exist. We try
to give useful error messages in those cases.
The main impact here is that we force lots of tests
to explicitly stub `length`.
Also, we can't do equality checks on zjquery
objects any more due to the proxy object, but the
easy workaround is to compare selectors. (This
is generally an unnecessary technique, anyway.)
The proxy wrapper is fairly straightforward, and
we just have a few special cases for things like
"inspect" that happen when you try to print out
objects.
We no longer store handlers as an array of functions,
and instead we assume that code will only ever set up
one handler per sel/event or sel/event/child. This is
almost always a sane policy for the app itself.
We also try to improve error handling when devs write
incorrect tests.
The only tests that required changes here are the
activity tests, which were a little careless about how
data got reset between tests.
Apparently, we didn't have one of these, and thus had a moderate
number of generally very old violations in the codebase. Fix this and
clear the ones that exist..
Adds box-shadow to `#searchbox` when either `#search_query` or any
of the pills have focus. Uses jquery instead of pure css as the
`:focus` event occurs on `#search_query`, while we want to add
box-shadow to `#searchbox`. This could have been done with
`:focus-within` CSS selector, but it is not supported in IE or Opera.
`#search_query` already had an onfocus/focusout listener, adding
listeners to `#searchbox.pills` for those events wouldn't have worked
as you don't want the focusout event to fire when the focus shifts
from input to pill.
Also adds `focusin`, `focusout` and `css()` to zjquery. `css` is
same as `val`, except it returns an empty object in case of no value
instead of an empty string. I don't think `css()` is valid syntax
in actual jquery.
The reason to add this api is that many times some elements are
already used/cached and then their value interfere/exists in
other tests which gives false results.
This is preparation for our migration of our JS pipeline to webpack,
which includes as part of the process a hack of exporting globals via
the window object.
We have less urgency to test all templates now. The
most common error is probably unbalanced tags, and our
python-based template checker catches those problems
pretty well.
It's still possible to create bad templates, of course,
but the node tests have never been super deep at finding
semantic errors.
Anders Kaseorg
Steve Howell
Priyank Patel
Aman Agrawal
Rohitt Vashishtha
Mohit Gupta
Sara Gulotta
Steve Howell
Stefan Weil
Anders Kaseorg
Greg Price
Tim Abbott
Thomas Ip
Vishnu Ks
Puneeth Chaganti
Cynthia Lin
Shubham Padia
Shubham Dhama