Skip to content
This repository has been archived by the owner on Apr 15, 2019. It is now read-only.

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

Merged
merged 6 commits into from
Feb 26, 2018
Merged

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

merged 6 commits into from
Feb 26, 2018

Conversation

shuse2
Copy link
Contributor

@shuse2 shuse2 commented Feb 14, 2018 •
edited by willclarktech
Loading

Closes #448

@shuse2 shuse2 self-assigned this Feb 14, 2018
willclarktech
willclarktech previously requested changes Feb 15, 2018
View reviewed changes
test/setup.js Outdated

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

Choose a reason for hiding this comment

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

Prefer using const and .some.

test/setup.js Outdated
// See https://github.com/shouldjs/should.js/issues/41
Object.defineProperty(global, 'should', { value: should });

[sinonChai, chaiAsPromised].forEach(plugin => chai.use(plugin));
Copy link
Contributor

Choose a reason for hiding this comment

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

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

test/setup.js Outdated
this.assert(
result,
'expected #{this} to match at least once',
'expected #{this} to not to match',
Copy link
Contributor

Choose a reason for hiding this comment

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

not to match

test/setup.js Outdated
const expected = Buffer.from(actual, 'hex').toString('hex');
this.assert(
expected === actual,
'expected #{this} to be a hexString',
Copy link
Contributor

Choose a reason for hiding this comment

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

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

test/steps/api/3_then.js Outdated
@@ -66,7 +66,7 @@ export function itShouldBroadcastTheTransaction() {

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

Choose a reason for hiding this comment

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

.should.eventually.eql

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we need be verb in natural language?

test/steps/config/3_then.js Outdated
@@ -42,7 +42,7 @@ export function itShouldUpdateTheConfigVariableToBoolean() {

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

Choose a reason for hiding this comment

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

As above.

test/steps/general/3_then.js Outdated
return testFunction.should
.throw()
.and.has.property('message')
.which.include(message);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

test/steps/general/3_then.js Outdated
return testFunction.should
.throw()
.and.has.property('message')
.which.include(message);
Copy link
Contributor

Choose a reason for hiding this comment

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

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?

test/steps/general/3_then.js Outdated
err.should.be.instanceOf(Error);
err.should.have.property('name').which.equal('FileSystemError');
return err.should.have.property('message').which.include(message);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar suggestion about adding a method to chai.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

test/steps/general/3_then.js
}

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

Choose a reason for hiding this comment

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

This is another downside to switching to chai.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@willclarktech willclarktech mentioned this pull request Feb 22, 2018
Closed
10 tasks
@shuse2 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 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
willclarktech
willclarktech reviewed Feb 26, 2018
View reviewed changes
Copy link
Contributor

@willclarktech willclarktech left a comment

Choose a reason for hiding this comment

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

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

test/steps/crypto/3_then.js Outdated
@@ -215,12 +215,12 @@ export function theDecryptedMessageShouldBeReturned() {

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

Choose a reason for hiding this comment

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

Prefer .equal where possible.

test/steps/general/3_then.js Outdated
@@ -82,13 +86,17 @@ export function itShouldRejectWithTheErrorMessage() {
export function itShouldRejectWithFileSystemErrorAndMessage() {
const { returnValue } = this.test.ctx;
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));
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer direct return without function body.

test/steps/general/3_then.js Outdated
}

export function itShouldRejectWithValidationErrorAndMessage() {
const { returnValue } = this.test.ctx;
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));
Copy link
Contributor

Choose a reason for hiding this comment

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

As above.

@willclarktech 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 willclarktech merged commit ee93414 into 1.0.0 Feb 26, 2018
@willclarktech willclarktech deleted the 426-change_testing_suite branch February 26, 2018 16:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@willclarktech willclarktech willclarktech approved these changes

Assignees

@shuse2 shuse2

Labels
✅ ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@shuse2 @willclarktech