Uh oh!
There was an error while loading. Please reload this page.
ConnectButton Tests using Jest - #378
Conversation
| } | ||
| /** | ||
| * Will check whether connection requests are equal to or greater then 400 |
| * Will check whether connection requests are equal to or greater then 400 | ||
| * @returns {boolean} true if connection error | ||
| */ | ||
| isConnectionError() { |
| * @returns {boolean} true if status is loading | ||
| */ | ||
| loadingStatus() { | ||
| return (isLoading(this.props.connectRequest.status) || isLoading(this.props.saveConnectionsRequest.status)); |
There was a problem hiding this comment.
isLoading is only used here. I'd get rid of isLoading and rename loadingStatus to isLoading (it's confusing to have two function so similar in name and purpose)
| * Checks whether the connection request status (HTTP) is greater or equal to 200 and less then 300 | ||
| * @returns {boolean} true if connection request status >=200 and <300 | ||
| */ | ||
| isValidConnection() { |
| * Checks whether connection status is defined | ||
| * @returns {boolean} true if connection status is invalid | ||
| */ | ||
| isInvalidConnectionStatus() { |
There was a problem hiding this comment.
This one if weird. The function name implies that it checks whether the connection failed, but the code checks we're still waiting for a response to out connection request.
I also suspect this may be the source of bug #372 (see my comment below; I'll test if I'm right).
| "style-loader": "^0.19.0", | ||
| "webpack": "^3.8.1", | ||
| "yamljs": "^0.3.0" | ||
| "yamljs": "^0.3.0", |
There was a problem hiding this comment.
we keep the dependencies in alphabetical order
| @@ -0,0 +1,289 @@ | |||
| jest.dontMock('../../../../../app/components/Settings/ConnectButton/ConnectButton.react.js'); | |||
There was a problem hiding this comment.
Are we affected by this issue with unmocked imports?
There was a problem hiding this comment.
No. Jest mocks modules by default. I specify that it should not mock this component so that we can test the component. This is a pattern that I have used before on other projects and seems to have worked
There was a problem hiding this comment.
I understand. What I meant is whether we need to use unmock instead.
There was a problem hiding this comment.
@n-riesco It looks like unmock is now the recommended approach. Here is a link to a link on SO (https://stackoverflow.com/questions/36571357/difference-between-unmock-and-dontmock-in-jest). I have updated the test and and added it to the PR
| "jest":"^22.3.0", | ||
| "babel-jest":"latest", | ||
| "react-test-renderer":"latest", | ||
| "react-addons-test-utils": "latest", |
There was a problem hiding this comment.
I think we don't need react-addons-test-utils, as we're using React 15.5
| "yamljs": "^0.3.0" | ||
| "yamljs": "^0.3.0", | ||
| "jest":"^22.3.0", | ||
| "babel-jest":"latest", |
| describe('Connect Button Tests', () => { | ||
| beforeAll(() => { | ||
| // Setup the test |
| import Adapter from 'enzyme-adapter-react-15'; | ||
| describe('Connect Button Tests', () => { | ||
| configure({ adapter: new Adapter() }); | ||
| }); | ||
| it('Should verify Connection Request Error', () => { |
There was a problem hiding this comment.
s/Should/should/ so that it reads "Connect Button should verify Connection Request Error".
Or even better "Connect Button should check for connection errors"
| }); | ||
| it('Should verify Connection Request Error', () => { | ||
| expect(ConnectButton).toBeDefined(); |
There was a problem hiding this comment.
no need for this check (import would've failed before)
| editMode={editMode} | ||
| />); | ||
| expect(button).toBeDefined(); |
| />); | ||
| expect(button).toBeDefined(); | ||
| expect(button.instance().isConnectionError()).toBeTruthy(); |
There was a problem hiding this comment.
I'd tighten this test: .toBe(true)
| }); | ||
| it('Should verify Save Connections Request Error', () => { | ||
| expect(ConnectButton).toBeDefined(); |
| editMode={editMode} | ||
| />); | ||
| expect(button).toBeDefined(); |
| />); | ||
| expect(button).toBeDefined(); | ||
| expect(button.instance().isConnectionError()).toBeTruthy(); |
| }); | ||
| it('Should verify Save Connections Request without Error', () => { | ||
| expect(ConnectButton).toBeDefined(); |
| editMode={editMode} | ||
| />); | ||
| expect(button).toBeDefined(); |
| />); | ||
| expect(button).toBeDefined(); | ||
| expect(button.instance().isConnectionError()).toBeFalsy(); |
| }); | ||
| it('Should verify loading connection request loading status', () => { | ||
| expect(ConnectButton).toBeDefined(); |
| editMode={editMode} | ||
| />); | ||
| expect(button).toBeDefined(); |
| />); | ||
| expect(button).toBeDefined(); | ||
| expect(button.instance().loadingStatus()).toBeTruthy(); |
| }); | ||
| it('Should verify loading save connections request loading status', () => { | ||
| expect(ConnectButton).toBeDefined(); |
| editMode={editMode} | ||
| />); | ||
| expect(button).toBeDefined(); |
| />); | ||
| expect(button).toBeDefined(); | ||
| expect(button.instance().loadingStatus()).toBeTruthy(); |
| }); | ||
| it('Should verify not loading status', () => { | ||
| expect(ConnectButton).toBeDefined(); |
| }); | ||
| it('Should connected button with error due to connect request error', () => { | ||
| expect(ConnectButton).toBeDefined(); |
| editMode={editMode} | ||
| />); | ||
| expect(button).toBeDefined(); |
| }); | ||
| it('Should connected button with error due to connect request error', () => { | ||
| expect(ConnectButton).toBeDefined(); |
| editMode={editMode} | ||
| />); | ||
| expect(button).toBeDefined(); |
| * The following is the Connect Button which triggers the connection | ||
| * @param {function} connect - Connect function | ||
| * @param {object} connectRequest - Connection Request | ||
| * @param {number || string} connectRequest.status -- 400 or loading |
There was a problem hiding this comment.
Also * @param {Error} [connectRequest.error]
| * @param {object} connectRequest - Connection Request | ||
| * @param {number || string} connectRequest.status -- 400 or loading | ||
| * @param {object} saveConnectionsRequest - Saved Connection Request | ||
| * @param {number || string } saveConnectionsRequest.status -- 400 or loading |
There was a problem hiding this comment.
Also * @param {Error} [saveConnectionsRequest.error]
| const isLoading = (status) => status === 'loading'; | ||
| /** |
There was a problem hiding this comment.
This JSDoc comment should go above static propTypes
| * @param {object} saveConnectionsRequest - Saved Connection Request | ||
| * @param {number || string } saveConnectionsRequest.status -- 400 or loading | ||
| * @param {boolean} editMode - Enabled if Editting credentials | ||
| * @returns {ConnectButton} |
| buttonText = 'Connected'; | ||
| } | ||
| } else if (!connectRequest.status) { | ||
| } else if (this.isInvalidConnectionStatus()) { |
There was a problem hiding this comment.
The return below wasn't properly indented. Could you fix it, please?
@shannonlal A very comprehensive set of specs! I find the logic in /** * @returns {boolean} true if waiting for a response to a connection request */isConnecting(){returnthis.props.connectRequest.status==='loading';}/** * @returns {boolean} true if successfully connected to database */isConnected(){conststatus=Number(this.props.connectRequest.status);return(status>=200||status<300);}/** * @returns {boolean} true if successfully connected to database */connectionFailed(){returnNumber(this.props.connectRequest.status)>=400;}/** * @returns {boolean} true if waiting for a response to a save request */isSaving(){returnthis.props.saveConnectionsRequest.status==='loading';}/** * @returns {boolean} true if connection has been saved */isSaved(){conststatus=Number(this.props.saveConnectionsRequest.status);return(status>=200||status<300);}/** * @returns {boolean} true if successfully connected to database */saveFailed(){returnNumber(this.props.saveConnectionsRequest.status)>=400;}render(){const{
connect,
connectRequest,
saveConnectionsRequest,
editMode
}=this.props;letbuttonText;letbuttonClick=()=>{};leterror=null;if(!editMode){buttonText='Connected';}elseif(this.isConnecting()||this.isSaving()){buttonText='Connecting...';}elseif(this.connectionFailed()||this.saveFailed()){buttonText='Connect';buttonClick=connect;constconnectErrorMessage=pathOr(null,['content','error'],connectRequest);constsaveErrorMessage=pathOr(null,['content','error','message'],saveConnectionsRequest);constgenericErrorMessage='Hm... had trouble connecting.';consterrorMessage=connectErrorMessage||saveErrorMessage||genericErrorMessage;error=<divclassName={'errorMessage'}>{errorMessage}</div>;}elseif(this.isConnected()&&this.isSaved()){buttonText='Save changes';buttonClick=connect;}else{buttonText='Connect';buttonClick=connect;}return(<divclassName={'connectButtonContainer'}><buttonid="test-connect-button"onClick={buttonClick}>{buttonText}</button>{error}</div>);} |
shannonlal
commented
Feb 20, 2018
@n-riesco I don't think your logic flow worked for the Connect Button. I will revisit this and step through it to get it working again. Keep you posted |
shannonlal
commented
Feb 20, 2018
@n-riesco I think I fixed most of the changes you recommended. I had to modify the logic flow a little bit for the Button Connect to get it to work but I tested it and it seems to be working. Please have a look and let me know if you think this makes sense |
Merge branch 'upstream-master' into Issue-362-ConnectButton-Test
shannonlal
commented
Feb 20, 2018
@n-riesco It looks like there is an issue with the ElasticSearch spec. I don't think it is related to anything I committed in. Here is the error:
Would you mind having a look at the error to let me know what might be causing the issue |
Merge branch 'upstream-master' into Issue-362-ConnectButton-Test
| } else { | ||
| } else if (this.isConnected()) { | ||
| buttonText = 'Save changes'; |
There was a problem hiding this comment.
'Save changes' should be shown only after we have successfully connected to a DB (i.e. this.isConnected() is true) and we have pressed Edit Credentials (i.e. editMode is true).
Also, see that editMode is false only if we have successfully connected to a DB (i.e. if editMode is false, then this.isConnected() is true). This is the reason why I wrote first if (!editMode) {...}`.
n-riesco
commented
Feb 20, 2018
What was the mistake? |
n-riesco
commented
Feb 20, 2018
The tests are failing because authentication is now enabled in the elasticsearch server. I've opened PR #382 to disable the tests in CircleCI against the elasticsearch server (we still have the tests against the mocked servers). |
n-riesco
commented
Feb 20, 2018
shannonlal
commented
Feb 21, 2018
@n-riesco I did some more testing and understand how the edit button should be working. I implemented the Connect Button as you suggested and modified the Jest Test cases to work accordingly. If you have a chance to look this over I would appreciate it. |
n-riesco
commented
Feb 21, 2018
@shannonlal LGTM 💃 Please, wait until #382 is merged, so that we can test this PR on CircleCI. |
The following is a first stab at the unit testing the front end components using Jest. The goal is to help document the front end and resolve some of the UI issues. I am hoping by breaking this down into small Unit Tests it will make it easy to see the expected behaviour of each of the components. I welcome any and all comments for this.
@n-riesco If you get a chance could you have a look at this. Are you the best person to review this or should I run this by someone else?