Change testing suite to follow lisk standards - Closes #448 #437

shuse2 · 2018-02-14T19:22:38Z

Closes #448

willclarktech · 2018-02-15T11:46:14Z

test/setup.js

+
+	new Assertion(obj).to.be.an('array');
+	let result = false;
+	obj.forEach(val => {


Prefer using const and .some.

willclarktech · 2018-02-15T11:48:31Z

test/setup.js

-// See https://github.com/shouldjs/should.js/issues/41
-Object.defineProperty(global, 'should', { value: should });
+
+[sinonChai, chaiAsPromised].forEach(plugin => chai.use(plugin));


Might be neater to use just .forEach(chai.use)?

willclarktech · 2018-02-15T11:48:42Z

test/setup.js

+	this.assert(
+		result,
+		'expected #{this} to match at least once',
+		'expected #{this} to not to match',


not to match

willclarktech · 2018-02-15T11:48:58Z

test/setup.js

+	const expected = Buffer.from(actual, 'hex').toString('hex');
+	this.assert(
+		expected === actual,
+		'expected #{this} to be a hexString',


We don't need camelcase in the user-facing message.

willclarktech · 2018-02-15T11:50:06Z

test/steps/api/3_then.js

 export function itShouldResolveToTheAPIResponse() {
 	const { returnValue, apiResponse } = this.test.ctx;
-	return returnValue.should.be.fulfilledWith(apiResponse);
+	return returnValue.should.be.eventually.eql(apiResponse);


.should.eventually.eql

I think we need be verb in natural language?

willclarktech · 2018-02-15T11:50:46Z

test/steps/config/3_then.js

 export function itShouldResolveToTheConfig() {
 	const { returnValue, config } = this.test.ctx;
-	return returnValue.should.be.fulfilledWith(config);
+	return returnValue.should.be.eventually.eql(config);


willclarktech · 2018-02-15T11:52:35Z

test/steps/general/3_then.js

+	return testFunction.should
+		.throw()
+		.and.has.property('message')
+		.which.include(message);


All these testFunction.should.throw()s seem redundant. It's awkward, but I think it would be better to store that in a variable and make further assertions against it.

willclarktech · 2018-02-15T11:53:57Z

test/steps/general/3_then.js

+	return testFunction.should
+		.throw()
+		.and.has.property('message')
+		.which.include(message);


Let's extract the common functionality into a helper function, so these then steps can be cleaner at least. Can we even add a method to chai?

willclarktech · 2018-02-15T11:54:27Z

test/steps/general/3_then.js

+		err.should.be.instanceOf(Error);
+		err.should.have.property('name').which.equal('FileSystemError');
+		return err.should.have.property('message').which.include(message);
+	});


Similar suggestion about adding a method to chai.

great idea! it's much cleaner.
I will add customError check function

willclarktech · 2018-02-15T11:59:23Z

test/steps/general/3_then.js

 export function itShouldReturnNull() {
 	const { returnValue } = this.test.ctx;
-	return should(returnValue).be.null();
+	return should.equal(returnValue, null);


This is another downside to switching to chai.

But I guess it'll be solved when we switch to expect.

yes, it will.
I almost tried to port except to global, too... lol

Addressed

willclarktech

README.md then step should be updated.
Prettier should be run on all files.

willclarktech · 2018-02-26T16:00:53Z

test/steps/crypto/3_then.js

 export function itShouldResolveToThePassphrase() {
 	const { returnValue, passphrase } = this.test.ctx;
-	return returnValue.should.be.fulfilledWith(passphrase);
+	return returnValue.should.eventually.eql(passphrase);


Prefer .equal where possible.

willclarktech · 2018-02-26T16:02:49Z

test/steps/general/3_then.js

 	const message = getFirstQuotedString(this.test.title);
-	return returnValue.should.be.rejectedWith(new FileSystemError(message));
+	return returnValue.should.be.rejected.then(err => {
+		return err.should.be.customError(new FileSystemError(message));


Prefer direct return without function body.

willclarktech · 2018-02-26T16:03:00Z

test/steps/general/3_then.js

 	const message = getFirstQuotedString(this.test.title);
-	return returnValue.should.be.rejectedWith(new ValidationError(message));
+	return returnValue.should.be.rejected.then(err => {
+		return err.should.be.customError(new ValidationError(message));


shuse2 added the 👁️ pending review label Feb 14, 2018

shuse2 self-assigned this Feb 14, 2018

shuse2 force-pushed the 426-change_testing_suite branch from d866859 to 2dd5643 Compare February 14, 2018 19:30

willclarktech previously requested changes Feb 15, 2018

View reviewed changes

willclarktech added 🏗️ in progress and removed 👁️ pending review labels Feb 15, 2018

shuse2 added 👁️ pending review and removed 🏗️ in progress labels Feb 15, 2018

willclarktech mentioned this pull request Feb 22, 2018

Apply lisk-template standards #426

Closed

10 tasks

shuse2 changed the title ~~Change testing suite to follow lisk standards - Closes #426~~ Change testing suite to follow lisk standards - Closes #448,3450 Feb 22, 2018

shuse2 changed the title ~~Change testing suite to follow lisk standards - Closes #448,3450~~ Change testing suite to follow lisk standards - Closes #448,#450 Feb 22, 2018

shuse2 force-pushed the 426-change_testing_suite branch from 55147d3 to c06eecb Compare February 26, 2018 15:56

shuse2 added 5 commits February 26, 2018 16:58

🆕 Change testing suite to follow standards

9dcefc0

♻️ Solve matchAny by creating function

7e26b8d

♻️ Add work around for cutom error check

cba00b3

♻️ change custom error handling to custom function

ba60a0b

♻️ Deleted non used code and fix test language

5182fed

shuse2 force-pushed the 426-change_testing_suite branch from c06eecb to 5182fed Compare February 26, 2018 15:58

willclarktech reviewed Feb 26, 2018

View reviewed changes

willclarktech added 🏗️ in progress and removed 👁️ pending review labels Feb 26, 2018

♻️ Fix syntaxing and change eql to equal where it can

3c8a68f

shuse2 added 👁️ pending review and removed 🏗️ in progress labels Feb 26, 2018

willclarktech approved these changes Feb 26, 2018

View reviewed changes

willclarktech added ✅ ready and removed 👁️ pending review labels Feb 26, 2018

willclarktech changed the title ~~Change testing suite to follow lisk standards - Closes #448,#450~~ Change testing suite to follow lisk standards - Closes #448 Feb 26, 2018

willclarktech merged commit ee93414 into 1.0.0 Feb 26, 2018

willclarktech deleted the 426-change_testing_suite branch February 26, 2018 16:56

Change testing suite to follow lisk standards - Closes #448 #437

Change testing suite to follow lisk standards - Closes #448 #437

Uh oh!

Conversation

shuse2 commented Feb 14, 2018 • edited by willclarktech Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

willclarktech left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shuse2 commented Feb 14, 2018 •

edited by willclarktech

Loading