Skip to content

normalize getters and setters (fixes #3058) #3064

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.

Sign up for GitHub

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

Merged
boneskull merged 1 commit into from
Oct 27, 2017
Merged

normalize getters and setters (fixes #3058) #3064

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 9 additions & 3 deletions lib/context.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ Context.prototype.runnable = function (runnable) {
};

/**
* Set test timeout `ms`.
* Set or get test timeout `ms`.
*
* @api private
* @param {number} ms
Expand All @@ -51,18 +51,24 @@ Context.prototype.timeout = function (ms) {
* @return {Context} self
*/
Context.prototype.enableTimeouts = function (enabled) {
if (!arguments.length) {
return this.runnable().enableTimeouts();
}
this.runnable().enableTimeouts(enabled);
return this;
};

/**
* Set test slowness threshold `ms`.
* Set or get test slowness threshold `ms`.
*
* @api private
* @param {number} ms
* @return {Context} self
*/
Context.prototype.slow = function (ms) {
if (!arguments.length) {
return this.runnable().slow();
}
this.runnable().slow(ms);
return this;
};
Expand All @@ -78,7 +84,7 @@ Context.prototype.skip = function () {
};

/**
* Allow a number of retries on failed tests
* Set or get a number of allowed retries on failed tests
*
* @api private
* @param {number} n
Expand Down
10 changes: 5 additions & 5 deletions lib/runnable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -91,14 +91,14 @@ Runnable.prototype.timeout = function (ms) {
};

/**
* Set & get slow `ms`.
* Set or get slow `ms`.
*
* @api private
* @param {number|string} ms
* @return {Runnable|number} ms or Runnable instance.
*/
Runnable.prototype.slow = function (ms) {
if (typeof ms === 'undefined') {
if (!arguments.length || typeof ms === 'undefined') {
Copy link
Member

@boneskull boneskull Oct 10, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can drop the ms check

Copy link
Contributor Author

@makepanic makepanic Oct 11, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've initially removed the ms check but tests were failing here.
I'm unsure if removing the ms check will introduce a breaking change.

Copy link
Member

@boneskull boneskull Oct 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ugh, hum, let me think about this.

Copy link
Member

@boneskull boneskull Oct 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that test is invalid and should be removed, but it's also a breaking change, so let's leave it for now.

Copy link

@fesebuv fesebuv Oct 26, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason why you test fails, is because when you invoke run.slow(undefined); argument.length is equal to 1. Instead of checking the number of arguments passed, you should just check if ms is undefined. You can do something like this:

if(!ms) {
  ....
}

Copy link
Member

@boneskull boneskull Oct 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fesebuv that's correct. but we just want to leave as-is for now. there's no change necessary here, since it'll be breaking.

Copy link

@fesebuv fesebuv Oct 27, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gotcha!

return this._slow;
}
if (typeof ms === 'string') {
Expand Down Expand Up @@ -144,7 +144,7 @@ Runnable.prototype.isPending = function () {
};

/**
* Set number of retries.
* Set or get number of retries.
*
* @api private
*/
Expand All @@ -156,7 +156,7 @@ Runnable.prototype.retries = function (n) {
};

/**
* Get current retry
* Set or get current retry
*
* @api private
*/
Expand Down Expand Up @@ -242,7 +242,7 @@ Runnable.prototype.resetTimeout = function () {
};

/**
* Whitelist a list of globals for this test run.
* Set or get a list of whitelisted globals for this test run.
*
* @api private
* @param {string[]} globals
Expand Down
10 changes: 5 additions & 5 deletions lib/suite.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ Suite.prototype.clone = function () {
};

/**
* Set timeout `ms` or short-hand such as "2s".
* Set or get timeout `ms` or short-hand such as "2s".
*
* @api private
* @param {number|string} ms
Expand All @@ -114,7 +114,7 @@ Suite.prototype.timeout = function (ms) {
};

/**
* Set number of times to retry a failed test.
* Set or get number of times to retry a failed test.
*
* @api private
* @param {number|string} n
Expand All @@ -130,7 +130,7 @@ Suite.prototype.retries = function (n) {
};

/**
* Set timeout to `enabled`.
* Set or get timeout to `enabled`.
*
* @api private
* @param {boolean} enabled
Expand All @@ -146,7 +146,7 @@ Suite.prototype.enableTimeouts = function (enabled) {
};

/**
* Set slow `ms` or short-hand such as "2s".
* Set or get slow `ms` or short-hand such as "2s".
*
* @api private
* @param {number|string} ms
Expand All @@ -165,7 +165,7 @@ Suite.prototype.slow = function (ms) {
};

/**
* Sets whether to bail after first error.
* Set or get whether to bail after first error.
*
* @api private
* @param {boolean} bail
Expand Down
12 changes: 12 additions & 0 deletions test/unit/context.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,18 @@ describe('methods', function () {
});
});

describe('slow()', function () {
it('should return the slow', function () {
expect(this.slow()).to.equal(75);
});
});

describe('enableTimeouts()', function () {
it('should return the enableTimeouts', function () {
expect(this.enableTimeouts()).to.equal(true);
});
});

describe('retries', function () {
it('should return the number of retries', function () {
expect(this.retries()).to.equal(-1);
Expand Down