Skip to content
This repository has been archived by the owner on Jun 27, 2021. It is now read-only.

Write test for utility functions #15

Open
wants to merge 19 commits into
base: next
Choose a base branch
from
Open

Conversation

luann112
Copy link

No description provided.

@lh0x00 lh0x00 added @nuz/utils Issues of @nuz/utils good first issue Good for newcomers labels May 24, 2020
@lh0x00 lh0x00 added this to In progress in First version release via automation May 24, 2020
@lh0x00 lh0x00 self-requested a review May 24, 2020 17:29
@lh0x00
Copy link
Member

lh0x00 commented May 24, 2020

Hi @luann112 , thanks for this PR.
I see the CI is not passed, please check it.

@lh0x00 lh0x00 removed the good first issue Good for newcomers label May 25, 2020
})

it('Should return a boolean value', async () => {
const result = await checkIsUrlOk('fb.com')
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 mock for this effect? don't call to network!

import compareFilesByHash from './compareFilesByHash'

describe('compareFilesByHash', () => {
it('Exported as default', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Don't need this case, should you remove:

import * as func from './compareFilesByHash'

import deferedPromise from './deferedPromise'

describe('deferedPromise', () => {
it('Exported as default', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Please handle (remove) as the case above


describe('ensureOrigin', () => {
it('Exported as default', () => {
expect(func.default).toBeDefined()
Copy link
Member

Choose a reason for hiding this comment

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

This is the same

})

it('Should match to snapshot', () => {
expect(deferedPromise()).toMatchSnapshot()
Copy link
Member

Choose a reason for hiding this comment

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

Please check the prototype instead of using snapshot for this case

})

it('Should match to the snapshot', () => {
const path = require.resolve('./hashFile')
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 mock or stub test file?

@@ -0,0 +1,23 @@
import * as func from './jsonHelpers'
Copy link
Member

Choose a reason for hiding this comment

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

Use jsonHelpers instead of func

@@ -0,0 +1,31 @@
import * as func from './linkedUrls'
Copy link
Member

Choose a reason for hiding this comment

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

Use linkedUrls instead of func

@@ -0,0 +1,15 @@
import * as func from './checkIsProductionMode'
Copy link
Member

Choose a reason for hiding this comment

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

Use checkIsProductionMode insead of func.
With a file export default only don't need to use:

import * as func from './checkIsProductionMode'

Use below:

import checkIsProductionMode from './checkIsProductionMode'

const fileA = require.resolve('./compareFilesByHash')
const fileB = require.resolve('./deferedPromise')
expect(compareFilesByHash(fileA, fileB)).toBe(false)
})
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 update compareFilesByHash and throw an error if files received not found?

@lh0x00
Copy link
Member

lh0x00 commented May 29, 2020

@luann112 I commented something, please help me update it. Don't forget to rebase with the next branch.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
@nuz/utils Issues of @nuz/utils
Projects
First version release
  
In progress
Development

Successfully merging this pull request may close these issues.

None yet

2 participants