-
Notifications
You must be signed in to change notification settings - Fork 0
Feature/CO-2036 unit tests #108
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
Conversation
What's the issue? I ran |
This PR is ready to be reviewed. I resolved the issue that I mentioned in teams. |
src/tests/unit/onbase-dao.test.js
Outdated
import proxyquire from 'proxyquire'; | ||
import 'config'; | ||
import setCookie from 'set-cookie-parser'; | ||
import { createConfigStub } from './test-helpers'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please adjust your import order to adhere to our JS coding style guide.
src/tests/unit/onbase-dao.test.js
Outdated
chai.should(); | ||
chai.use(chaiAsPromised); | ||
|
||
describe('Test emails-dao', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is email-dao?
src/tests/unit/onbase-dao.test.js
Outdated
let stubAxios; | ||
let onbaseDao; | ||
let stubAxios2; | ||
let onbaseDao2; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please rename your variables and make them more descriptive.
src/tests/unit/onbase-dao.test.js
Outdated
createConfigStub(); | ||
stubAxios = sinon.stub().callsFake( | ||
() => fakeData, | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the purpose of breaking lines here? Again, please ensure all of your code follows our coding conversion and follows the style guide. There are lots of styling issues among your codes; I'm not going to reply to them all individually.
src/tests/unit/onbase-dao.test.js
Outdated
); | ||
onbaseDao2 = proxyquire('db/http/onbase-dao', { | ||
axios: stubAxios2, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you need all onbaseDao
/onbaseDao2
and stubAxios
/stubAxios2
for each test class? If not, why do you need to create all of them beforeEach
and restore all of them at afterEach
?
sinon.assert.calledOnce(stub); | ||
}; | ||
|
||
it('getAccessToken for valid profile should return a valid token', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if getAccessToken
got an invalid profile? and how about if the response is not 200?
Same comments with all of your other test cases. Besides successful/fulfilled cases, please also ensure they can catch the error as well.
onbaseDao.getDocumentsByIds('fake token', 'fake fbLb', 'fake id'), | ||
stubAxios, | ||
)); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Besides DAO tests, you also need to test the serializers. Also, I updated getAccessToken
function already to address users who might use the wrong cached credential, please make sure your code tested that part and reflect the change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Much better. I left a few comments but all are very minor and are styling related.
@@ -0,0 +1,36 @@ | |||
import chai from 'chai'; | |||
import proxyquire from 'proxyquire'; | |||
import { testMultipleResources, testSingleResource, createConfigStub } from './test-helpers'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A blank line is needed here for separating import groups.
src/tests/unit/test-helpers.js
Outdated
* Helper function for creating an error response for a web request. | ||
* | ||
* @param {number} code the HTTP status code of the error | ||
* @returns {object} an error resonse object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: response.
it('serializeDocument should properly serialize a single document (GET) in JSON API format', () => { | ||
const document = { | ||
id: 'fake id', | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor stuff, this fake document has been used multiple times, so it's better to store it in mock-data.js and reuse it.
const doc = { | ||
typeId: 'fake id', | ||
}; | ||
const result = serializeDocuments([doc, doc], ''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a minor stuff, since all the other fake stuff you named "fake something", should this be named in the same way?
@@ -0,0 +1,29 @@ | |||
import chai from 'chai'; | |||
import proxyquire from 'proxyquire'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A blank line is also needed here.
it('serializeKeywords should properly serialize keywords collection (GET) in JSON API format', () => { | ||
const updatedKeywordCollection = { | ||
id: 'fake id', | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing, I think this can be reused and stored in mock-data.js.
import proxyquire from 'proxyquire'; | ||
import 'config'; | ||
import setCookie from 'set-cookie-parser'; | ||
import { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A blank line is needed here.
src/tests/unit/onbase-dao.test.js
Outdated
id: 'fake id', | ||
items: [keywordType1, keywordType2], | ||
}, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since you have mock-data.js, let's move all these fake data to there.
}, | ||
}; | ||
|
||
const runErrorSuite = (testLabel, getter, errReturn, errThrowObj, errThrowMsg) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you also add document strings for this function as well?
const result = serializeKeywords(updatedKeywordCollection, { method: 'POST', query: 'fake' }); | ||
return testSingleResource(result); | ||
}); | ||
it('serializeKeywords should properly serialize keywords collection (GET) in JSON API format', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In "Test keywords-serializer" and "Test documents-serializer", GET and POST tests are actually exactly the same besides the input method. You should be able to combine them with different var and use template strings to format the testing titles as you did in https://github.com/osu-mist/onbase-docs-api/pull/108/files#diff-c1a7ac1090dae64f3f41dfa948d89eae526e17bdc44df6ae83f4e0e21b471b3cR66
No description provided.