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

increasing test coverage #1217

Merged
merged 18 commits into from
Nov 16, 2020
Merged

increasing test coverage #1217

merged 18 commits into from
Nov 16, 2020

Conversation

elizabeth-jimenez
Copy link

@elizabeth-jimenez elizabeth-jimenez commented Oct 26, 2020

Going to add a few more tests. Done

@elizabeth-jimenez elizabeth-jimenez added the WIP Work in progress label Oct 26, 2020
@@ -1,6 +1,4 @@
import { shallow } from 'enzyme';
import React from 'react';
import toJSON from 'enzyme-to-json';

Choose a reason for hiding this comment

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

You still need toJSON

Copy link
Author

Choose a reason for hiding this comment

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

😭

@elizabeth-jimenez
Copy link
Author

So this PR actually shows a lower coverage than dev lol because of our new React16 components(Bureau). I'm going to give that hook testing library another shot 🤞

@@ -29,6 +53,38 @@ describe('UserProfileGeneralInformationComponent', () => {
expect(toJSON(wrapper)).toMatchSnapshot();
});

// could not get these to work :[
Copy link

Choose a reason for hiding this comment

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

Those ids are generated. You'll need to stub the function that generates ids so that the output is consistent/static

Copy link
Author

Choose a reason for hiding this comment

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

oh yeaa, i remember something like generateShortID 🤔

@mjoyce91
Copy link

mjoyce91 commented Nov 2, 2020

very nice! even if it doesn't bring up the numbers, this makes our tests more comprehensive

Comment on lines 66 to 71
it('get Employee Profile URL', (done) => {
({ mock, spy } = spyMockAdapter({
url: 'HR/Employees/4/EmployeeProfileReportByCDO',
response: [200, { data: 'arraybuffer',
type: 'application/pdf' }],
})); mock();
Copy link
Author

Choose a reason for hiding this comment

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

@mjoyce91 couldn't figure it out. RIP. EmployeeProfileReportByCDO url and downloadPdfBlob() handle the request different than About. I'll take out for now. Just committed it to keep a history.

@elizabeth-jimenez
Copy link
Author

image

Comment on lines 67 to 73
({ mock, spy } = spyMockAdapter({
url: 'HR/Employees/4/EmployeeProfileReportByCDO',
response: [200, { data: 'arraybuffer',
type: 'application/pdf' }],
})); mock();

expectMockWasCalled({ spy, cb: done });
Copy link

Choose a reason for hiding this comment

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

You're never making the network request from the component - lines 67-71 only mock the network request, but they don't actually make the network request. So you'll need to call wrapper.instance().getEmployeeProfile()

Copy link
Author

Choose a reason for hiding this comment

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

Does mock() just create the mock network request?

Comment on lines 64 to 80
it('get Employee Profile URL', (done) => {
let mock;
let spy;
({ mock, spy } = spyMockAdapter({
url: 'HR/Employees/4/EmployeeProfileReportByCDO',
response: [200, { data: 'arraybuffer',
type: 'application/pdf' }],
})); mock();

const wrapper = shallow(<UserProfileGeneralInformation.WrappedComponent
userProfile={bidderUserObject}
/>);

wrapper.instance().getEmployeeProfile();

expectMockWasCalled({ spy, cb: done });
});
Copy link
Author

Choose a reason for hiding this comment

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

Did not work... 😭

Copy link
Author

Choose a reason for hiding this comment

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

also tried passing in getEmployeeProfile as a prop ()

Choose a reason for hiding this comment

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

Ah I see. Your URL on line 68 is wrong. It doesn't have a leading slash, but more importantly, EmployeeProfileReportByCDO doesn't get called by api(), which is what spyMockAdapter uses. It gets a URL returned by our API. We can skip this for now

Copy link
Author

Choose a reason for hiding this comment

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

I was like 🤔 hitting the store... but why lol
Dang, that leading slash always gets me
Woot! Will remove tomorrow! Thanks for checking it out 🎊

@elizabeth-jimenez
Copy link
Author

I saw this somewhere let [mock, spy, store] = Array(3); what magic is it?

@mjoyce91
Copy link

@elizabeth-jimenez just a one-line way of doing:

let mock
let spy
let store

@elizabeth-jimenez
Copy link
Author

@elizabeth-jimenez just a one-line way of doing:

let mock
let spy
let store

Ahh

@elizabeth-jimenez elizabeth-jimenez merged commit 83a97db into dev Nov 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants