Skip to content

Conversation

@nechaido
Copy link
Member

Refs: #145

  • move test cases to fixtures from tests
  • port RemoteError test to tap
  • remove mocha tests for RemoteError

@nechaido nechaido added the test label May 15, 2017
@nechaido nechaido mentioned this pull request May 15, 2017
7 tasks
@nechaido nechaido force-pushed the remote-error-test branch from a38e726 to eb0ed8d Compare May 18, 2017 15:31
@nechaido
Copy link
Member Author

@metarhia/jstp-core ping

const jstp = require('../../../');

module.exports = Object.keys(jstp)
.filter(key => key.startsWith('ERR_'))
Copy link
Member

Choose a reason for hiding this comment

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

These should be hardcoded instead of dynamically introspected.

Copy link
Member Author

Choose a reason for hiding this comment

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

@aqrln this test is for checking that every jstp error has a default message. Thus introspection is needed here in case someone adds new jstp error but forgets to add a default message.

@@ -0,0 +1,42 @@
'use strict';
Copy link
Member

Choose a reason for hiding this comment

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

I don't actually see any reason for the whole fixtures/remote-error-test-cases directory to exist. These files are not pure data, they contain quite a bit of imperative logic, and they are very small and isolated. I'd say their contents should be simply moved to the corresponding tests.

Copy link
Member Author

Choose a reason for hiding this comment

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

@aqrln Thank you for your review.
I have added changes due to it, although I do not see a lot of imperative logic:

  • scanning for default error messages
  • calling a RemoteError and TypeError constructor

Both moved to a related test.

@nechaido
Copy link
Member Author

@aqrln ping

Copy link
Member

@aqrln aqrln left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

nechaido added a commit that referenced this pull request May 26, 2017
PR-URL: #167
Refs: #145
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@nechaido
Copy link
Member Author

Landed in 5ea6001.

@nechaido nechaido closed this May 26, 2017
@nechaido nechaido deleted the remote-error-test branch June 2, 2017 23:59
belochub pushed a commit that referenced this pull request Jan 22, 2018
PR-URL: #167
Refs: #145
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
belochub pushed a commit that referenced this pull request Jan 22, 2018
PR-URL: #167
Refs: #145
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
@belochub belochub mentioned this pull request Jan 22, 2018
belochub pushed a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018
PR-URL: metarhia/jstp#167
Refs: metarhia/jstp#145
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
belochub pushed a commit to metarhia/mdsf that referenced this pull request Jul 19, 2018
PR-URL: metarhia/jstp#167
Refs: metarhia/jstp#145
Reviewed-By: Alexey Orlenko <eaglexrlnk@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants