Skip to content
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

passport spec #77

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions models/passport.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,10 @@ module.exports = (sequelize, DataTypes) => {

Passport.associate = function (models) {
Passport.belongsTo(models.User);
},
};

Passport.providerCode = function () {
return new Enum({ 'SLACK': 0, 'FIREBASE': 1, 'LOCAL': 2,});
return new Enum({ SLACK: 0, FIREBASE: 1, LOCAL: 2 });
};
return Passport;
};
97 changes: 77 additions & 20 deletions test/unit/models/passport.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,47 +5,56 @@ const fakeData = {
create: {
token: faker.name.findName(),
workspaceName: faker.name.findName(),
passwordHash: faker.internet.password(),
},
update: {
token: faker.name.findName(),
workspaceName: faker.name.findName(),
passwordHash: faker.internet.password(),
},
updateNewData: {
token: faker.name.findName(),
workspaceName: faker.name.findName(),
passwordHash: faker.internet.password(),
},
updateNewData2: {
token: faker.name.findName(),
workspaceName: faker.name.findName(),
passwordHash: faker.internet.password(),
},
findOne: {
token: 'faker.name.findName()',
workspaceName: faker.name.findName(),
passwordHash: faker.internet.password(),
},
findAll: [
{
token: 'faker.name.findName()',
workspaceName: faker.name.findName(),
passwordHash: faker.internet.password(),
},
{
token: 'faker.name.findName()',
workspaceName: faker.name.findName(),
passwordHash: faker.internet.password(),
},
{
token: 'faker.name.findName()',
workspaceName: faker.name.findName(),
passwordHash: faker.internet.password(),
},
],
destroy: {
token: faker.name.findName(),
workspaceName: faker.name.findName(),
passwordHash: faker.internet.password(),
},
keyword: {
token: 'faker.name.findName()',
},
};

describe(`models/${modelName}`, () => {
describe.only(`models/${modelName}`, () => {
before(() => {
});

Expand All @@ -54,10 +63,10 @@ describe(`models/${modelName}`, () => {

describe('Create model data', () => {
it('creates a data', async () => {
// TODO:
// TODO:
// 1. model.create(fakeData.create)...
// 2. use data from `fakeData.create`
let createdModel = null;
let createdModel = await models[modelName].create(fakeData.create);
Copy link
Member

Choose a reason for hiding this comment

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

Tomas 建議如果沒有重新賦值會使用 const 來宣告


Object.keys(fakeData.create).forEach(e => {
expect(createdModel[e]).to.equal(fakeData.create[e]);
Expand All @@ -77,23 +86,55 @@ describe(`models/${modelName}`, () => {
// 1. use model.find(modelId === updatedModel.id) then model.save();
// 2. use data from `fakeData.updateNewData`
// 3. use `findAndUpdatedModel` as target, `modelDataForUpdate.id` as condition.
let findAndUpdatedModel = null;

Object.keys(fakeData.updateNewData).forEach(e => {
expect(findAndUpdatedModel[e]).to.equal(fakeData.updateNewData[e]);
});
try {
let findAndUpdatedModel = await models[modelName].findOne({
Copy link
Contributor

Choose a reason for hiding this comment

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

前面是用 findOne
這邊卻用 findAndUpdateModel
唸起來怪怪的
他應該只是一個 userdata?

他回傳的應該還是個物件
只是有一個 wrapper 提供你直接修改 DB的街口
但是他本質還是 data

Copy link
Member

Choose a reason for hiding this comment

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

這裡的變數是我命名的QQ

因為這個單元測試有指定操作方法,這裏想表達的含義是該 model data 是需要使用 find 去做 update,所以我用這個 findAndUpdatedModel 表示該 model 是 find 之後 updated

不過命名的確有改空間

where: {
id: modelDataForUpdate.id,
},
});

if (findAndUpdatedModel) {
Copy link
Member

Choose a reason for hiding this comment

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

這邊就不用包 if 了,因為這是 unit test,所以 findAndUpdatedModel 一定不能是 null

包 if 反而如果有問題不能發現

findAndUpdatedModel.token = fakeData.updateNewData.token;
findAndUpdatedModel.workspaceName =
fakeData.updateNewData.workspaceName;
findAndUpdatedModel.passwordHash =
fakeData.updateNewData.passwordHash;
Copy link
Contributor

@horsekitlin horsekitlin Aug 21, 2018

Choose a reason for hiding this comment

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

這是測試建立 data 的麻?
為什麼這裡還會有修改 data 的行為?

L96 - L103 我應該會拉出去寫一個function
EX:
我還是會比較習慣用 update 去做

const updateData = (modal, {updateNewData}) => {
  const newModal = {...lmodal, ...updateNewData};
  return newModal.save();
}

Copy link
Member

Choose a reason for hiding this comment

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

QQ 這裡是我建好的 init data 缺了一個 notNull 欄位的資料,所以我請 @chenpion 直接補上

Copy link
Contributor

@horsekitlin horsekitlin Aug 21, 2018

Choose a reason for hiding this comment

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

了解!
不過 initial data 不是應該會在 beforeAll or beforeEach ?

Copy link
Member

Choose a reason for hiding this comment

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

正常是不會,但在測試 update() / destroy() 時會需要在 before 中建立,因為必須要先有資料才能做以上兩個操作

以這裡來說, @chenpion 這裡的變動是寫在 spec 最前端,所以應該比較沒有疑義?

參考:

Copy link
Contributor

Choose a reason for hiding this comment

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

了解


await findAndUpdatedModel.save();

Object.keys(fakeData.updateNewData).forEach(e => {
expect(findAndUpdatedModel[e]).to.equal(fakeData.updateNewData[e]);
});
}
} catch (error) {
console.log('find and save => ', error);
}
});

it('update a model with update method', async () => {
// TODO:
// 1. use model.update()...
// 2. use data from `fakeData.updateNewData2`
// 3. use `updateModel` as target, `modelDataForUpdate.id` as condition.
let updateModel = null;

Object.keys(fakeData.updateNewData2).forEach(e => {
expect(updateModel[e]).to.equal(fakeData.updateNewData2[e]);
});
try {
await models[modelName].update(fakeData.updateNewData2, {
where: {
id: modelDataForUpdate.id,
},
});

let updateModel = await models[modelName].findOne({
Copy link
Contributor

Choose a reason for hiding this comment

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

這一個 modelName 是一個常數
應該可以改成 PASSPORT_MODEL = 'Passport';
這樣後面再讀的時候才不用都拉到最上面去找這個 modelName 是什麼

Copy link
Member

Choose a reason for hiding this comment

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

這裏跟 user.spec 是一樣的原因,modelName 作用是讓大部分 spec 可以共用的 model 名稱參數,不過因為用途是常數,應該改為大寫沒錯

建議 @chenpion 改為 SPEC_MODEL_NAME

Copy link
Contributor

@horsekitlin horsekitlin Aug 21, 2018

Choose a reason for hiding this comment

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

了解
因為每個檔案都會定義自己的 SPEC_MODEL_NAME
共用的意義似乎不是太大 QQ

如果變數可以直接體現出這個 Model Name
review 上應該會比較輕鬆
不過這只是 SPEC 檔案就有分開了
應該也還好 XD

Copy link
Member

@iamcxa iamcxa Aug 21, 2018

Choose a reason for hiding this comment

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

因為我等於是寫了第一份 spec 之後,改這個 modelName 再複製出來,就可以讓單元測試的大部分程式碼共用於全部的 model,我再針對某些 model 彼此特殊的關係開細部測試規格給大家即可

而你說的直接從變數名稱上看到是哪個 model 這件事,因為以 passport.spec.js 這個檔案來說,它就是針對路徑 models/passport 這個 model 來做單元測試,我想應該識別度上沒有問題才是?

Copy link
Contributor

Choose a reason for hiding this comment

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

辨識度上應該還好
只是如果能夠更符合變數的命名
對review 或是之後承接的人會比較輕鬆一點

where: {
id: modelDataForUpdate.id,
},
});

Object.keys(fakeData.updateNewData2).forEach(e => {
expect(updateModel[e]).to.equal(fakeData.updateNewData2[e]);
});
} catch (error) {
console.log('update => ', error);
}
});
});

Expand All @@ -109,7 +150,11 @@ describe(`models/${modelName}`, () => {
// 1. use model.findOne...
// 2. use data from `fakeData.findOne`
// 3. use `findModel` as target, `modelDateForFind.id` as option.
let findModel = null;
let findModel = await models[modelName].findOne({
where: {
id: modelDateForFind.id,
},
});

Object.keys(fakeData.findOne).forEach(e => {
expect(findModel[e]).to.equal(fakeData.findOne[e]);
Expand All @@ -129,7 +174,7 @@ describe(`models/${modelName}`, () => {
// 1. use model.findAll...
// 2. use data from `fakeData.findAll`
// 3. use `findAllModel` as target.
let findAllModel = null;
let findAllModel = await models[modelName].findAll();

expect(findAllModel.length).to.greaterThan(fakeData.findAll.length - 1);
});
Expand All @@ -139,11 +184,15 @@ describe(`models/${modelName}`, () => {
// 1. use model.findAll...
// 2. use data from `fakeData.findAll`
// 3. use `findAllModel` as target, and use `fakeData.keyword` as option.
let findAllModel = null;
let findAllModel = await models[modelName].findAll({
Copy link
Contributor

Choose a reason for hiding this comment

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

同上
findModel 似乎應該是 function (尋找model)
他這邊卻是變數

where: fakeData.keyword,
});

expect(findAllModel.length).to.greaterThan(fakeData.findAll.length - 1);
Object.keys(fakeData.findAll).forEach(e => {
expect(findAllModel[e]).to.equal(fakeData.findAll[e]);
fakeData.findAll.forEach(i => {
Object.keys(i).forEach(e => {
expect(findAllModel[e]).to.equal(fakeData.findAll[e]);
});
});
});
});
Expand All @@ -160,8 +209,16 @@ describe(`models/${modelName}`, () => {
// 1. use model.destroy...
// 2. use data from `fakeData.findAll`
// 3. use `findAllModel` as target, and use `modelDataForDestroy.id` as option.
let deleteModel = null;
let findDeleteModel = null;
let deleteModel = await models[modelName].destroy({
where: {
id: modelDataForDestroy.id,
},
});
let findDeleteModel = await models[modelName].findOne({
where: {
id: modelDataForDestroy.id,
},
});
expect(deleteModel).to.equal(1);
expect(findDeleteModel).to.equal(null);
});
Expand Down
2 changes: 1 addition & 1 deletion test/unit/models/user.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ describe(`models/${modelName}`, () => {
});
});

describe.only('Associated model data', () => {
describe('Associated model data', () => {
it('Create a model with associated data using include', async () => {
// TODO:
// 1. create a User with 3 passports.
Expand Down