Skip to content

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

Merged
merged 7 commits into from
Jul 3, 2023
Merged

Feature/CO-2036 unit tests #108

merged 7 commits into from
Jul 3, 2023

Conversation

fernamig
Copy link
Contributor

@fernamig fernamig commented May 8, 2023

No description provided.

@tsoliangwu0130
Copy link
Member

tsoliangwu0130 commented May 8, 2023

What's the issue? I ran npm test but didn't see any error. Or is this PR ready to be reviewed?

@fernamig
Copy link
Contributor Author

This PR is ready to be reviewed. I resolved the issue that I mentioned in teams.

@fernamig fernamig requested a review from tsoliangwu0130 May 18, 2023 21:24
import proxyquire from 'proxyquire';
import 'config';
import setCookie from 'set-cookie-parser';
import { createConfigStub } from './test-helpers';
Copy link
Member

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.

chai.should();
chai.use(chaiAsPromised);

describe('Test emails-dao', () => {
Copy link
Member

Choose a reason for hiding this comment

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

What is email-dao?

let stubAxios;
let onbaseDao;
let stubAxios2;
let onbaseDao2;
Copy link
Member

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.

createConfigStub();
stubAxios = sinon.stub().callsFake(
() => fakeData,
);
Copy link
Member

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.

);
onbaseDao2 = proxyquire('db/http/onbase-dao', {
axios: stubAxios2,
});
Copy link
Member

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',
Copy link
Member

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,
));
});
Copy link
Member

@tsoliangwu0130 tsoliangwu0130 May 18, 2023

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.

Copy link
Member

@tsoliangwu0130 tsoliangwu0130 left a 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';
Copy link
Member

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.

* 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
Copy link
Member

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',
};
Copy link
Member

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], '');
Copy link
Member

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';
Copy link
Member

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',
};
Copy link
Member

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 {
Copy link
Member

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.

id: 'fake id',
items: [keywordType1, keywordType2],
},
};
Copy link
Member

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) => {
Copy link
Member

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', () => {
Copy link
Member

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

@fernamig fernamig merged commit 722fd9b into v2 Jul 3, 2023
@fernamig fernamig deleted the feature/CO-2036-unit-tests branch July 3, 2023 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants