New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Serialize server tests #2088

Closed
wants to merge 2 commits into
base: devel
from

Conversation

Projects
None yet
3 participants
@awwx
Contributor

awwx commented Apr 24, 2014

Queue server tests so that they run one at a time, even if multiple clients are simultaneously submitting tests.

Add a three minute timeout to server tests to avoid hanging the server if a test fails to complete.

Remove the async flag and implement Tinytest.add in terms of Tinytest.addAsync.

awwx added some commits Apr 22, 2014

Fix serializing server tests.
`Tinytest.add` is now implemented in terms of `Tinytest.addAsync`, and
the old `async` flag removed.
@mcbain

This comment has been minimized.

mcbain commented Apr 25, 2014

Why serialize test-runs on the server side?
IMHO there is no need to do this. Tests should not interact with each
other, not in the same run, nor in different runs.
For collections testing, just use random generated collection names.

2014-04-25 0:57 GMT+02:00 Andrew Wilcox notifications@github.com:

Queue server tests so that they run one at a time, even if multiple
clients are simultaneously submitting tests.

Add a three minute timeout to server tests to avoid hanging the server if
a test fails to complete.

Remove the async flag and implement Tinytest.add in terms of

Tinytest.addAsync.

You can merge this Pull Request by running

git pull https://github.com/awwx/meteor serialize-server-tests

Or view, comment on, or merge it at:

#2088
Commit Summary

  • Run server tests one at a time.
  • Fix serializing server tests.

File Changes

  • M packages/email/email.jshttps://github.com//pull/2088/files#diff-0(3)
  • M packages/email/email_tests.jshttps://github.com//pull/2088/files#diff-1(2)
  • M packages/mongo-livedata/allow_tests.jshttps://github.com//pull/2088/files#diff-2(3)
  • M packages/tinytest/tinytest.jshttps://github.com//pull/2088/files#diff-3(178)

Patch Links:

Reply to this email directly or view it on GitHubhttps://github.com//pull/2088
.

@awwx

This comment has been minimized.

Contributor

awwx commented Apr 25, 2014

Why serialize test-runs on the server side?

Having multiple tests be able to run simultaneously is a nice ideal, and wouldn't be hard if collections were the only thing being tested, but Meteor has lots of global state. See for example Accounts.config and OAuthEncryption.loadKey.

Because some tests make connections to the server, it wouldn't be sufficient to merely have global state scoped to the test, we'd need to have connections from different tests run with the global state of the particular test as well. By that point we'd be essentially creating a server instance with its own global state for each test.

As it turns out server tests aren't the slow part of the test runs, so it isn't a big deal to serialize tests on the server, and much simpler.

If sometime someone does need to run multiple server tests simultaneously, there's an easy way to do that: run multiple Node instances to run tests on.

In the end, enabling multiple server tests to run simultaneously in a single Node instance safely would require a lot of special purpose code that would only be used while testing. And the failure mode of a test which isn't successfully converted to run safely interleaved with other tests (or other instances of itself) is particularly pernicious: the test runs fine almost all the time, and then occasionally you get an test failure that is very hard to track down and debug because merely seeing the test run successfully doesn't tell you whether you've fixed the bug or not. The result is a lot of time spent debugging tests, time that would be more usefully used debugging Meteor.

n1mmy added a commit that referenced this pull request May 1, 2014

@n1mmy

This comment has been minimized.

Member

n1mmy commented May 1, 2014

Merged! Thanks, @awwx.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment