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

WIP ⚠ configure jest with enzyme #105

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

rady-ben
Copy link
Contributor

@rady-ben rady-ben commented Sep 3, 2021

  • add new jest config
  • add enzyme config
  • test the render of the FileForm component

This change is Reviewable

@@ -67,6 +67,7 @@
"babel-jest": "24.9.0",
"babel-loader": "^8.0.6",
"babel-plugin-istanbul": "6.0.0",
"check-prop-types": "^1.1.2",
Copy link
Contributor

Choose a reason for hiding this comment

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

TODO: discuss with the team:
Seems like a helpful idea to verify prop-types on PUBLIC libraries.

@@ -59,6 +59,9 @@ FileForm.propTypes = {
submitText: PropTypes.string,
/** Function run when submit button is clicked */
onSubmit: PropTypes.func.isRequired,
branch: PropTypes.string,
filepath: PropTypes.string,
defaultContent: PropTypes.string,
Copy link
Contributor

@ancientTexts-net ancientTexts-net Sep 8, 2021

Choose a reason for hiding this comment

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

NOTE: there were a few missing proptypes 👍

@ancientTexts-net
Copy link
Contributor


jest.enzyme.config.js, line 1 at r1 (raw file):

module.exports = {

For now, I think there is a different config for these "new" tests.

If this PR is going to be approved, we can probably refactor a single jest.config.js and

@ancientTexts-net
Copy link
Contributor


package.json, line 32 at r1 (raw file):

    "test:unit": "NODE_ENV=test jest ./src/core --coverage && cat ./coverage/lcov.info | coveralls",
    "test": "nyc --exclude-after-remap=false npm run test:e2e",
    "testJestEnzyme": "jest --watch -c ./jest.enzyme.config.js",

For now, there are two ways to invoke jest.

But if this PR is going to be approved, maybe we can refactor into a single (combined) script

@ancientTexts-net
Copy link
Contributor

ancientTexts-net commented Sep 8, 2021


mocks/fileMock.js, line 1 at r1 (raw file):

module.exports = 'test-file-stub';

see identity-obj-proxy 👍

@ancientTexts-net
Copy link
Contributor


src/components/file/FileForm.js, line 22 at r1 (raw file):

    <Paper 
      style={{ marginBottom: '1em', padding: '1.3em' }}
      data-test="component-fileForm"

Several data-test elements added. 👍

@ancientTexts-net
Copy link
Contributor

ancientTexts-net commented Sep 8, 2021


mocks/FileForm.test.js, line 27 at r1 (raw file):

const findByAttribute = (wrapper, attribute) => wrapper.find(`[data-test='${attribute}']`)

// render tests

TODO: can we look at these tests?

They look helpful to me.

  • is this a good pattern for tests in general?
  • do they offer meaningful coverage of statements?
  • are they specific enough?
  • do they cover possible edge cases?

Copy link
Contributor

@ancientTexts-net ancientTexts-net left a comment

Choose a reason for hiding this comment

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

Reviewed all commit messages.
Reviewable status: 0 of 6 files reviewed, 5 unresolved discussions (waiting on @rady-ben)

@rady-ben
Copy link
Contributor Author

rady-ben commented Sep 8, 2021

combined

package.json, line 32 at r1 (raw file):

    "test:unit": "NODE_ENV=test jest ./src/core --coverage && cat ./coverage/lcov.info | coveralls",
    "test": "nyc --exclude-after-remap=false npm run test:e2e",
    "testJestEnzyme": "jest --watch -c ./jest.enzyme.config.js",

For now, there are two ways to invoke jest.

But if this PR is going to be approved, maybe we can refactor into a single (combined) script

I added the new script because I saw this error message when I run the existing script error Command failed with exit code 1. and the script stops and exits. and there in no watch, jest doesn't refreshes when I updated the tests that’s why I had the idea to add new command with the new tests.
an other reason for separating is that the existing test concern the differents APIs, however the news ones concern the react components

@ancientTexts-net ancientTexts-net changed the title configure jest with enzyme WIP ⚠ configure jest with enzyme Sep 10, 2021
const previewButton = findByAttribute(wrapper, 'previewButton');
previewButton.simulate('click');
expect(mockSetPreview).toBeCalledWith(false);
// NEED HELP
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ancientTexts-net
in this function I am testing the initial value to be true and the and when the user click on the button I test that that the setPreview is call with the false as param. which describe the switch from true to false.
but I don't know how to set realy the state on the component to false so I will be able to test the switch from false to true.
I found that in a class component we use something like this
wrapper.instance().setPreview(false)
I am looking for an equivalent for the hooks

onEdit={setMarkdown}
editable={!!authentication}
editable={isAuthenticated}
data-test="blockEditable"
Copy link
Contributor Author

@rady-ben rady-ben Sep 11, 2021

Choose a reason for hiding this comment

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

@ancientTexts-net
how can I simulate the onEdit event with jest BlockEditable.simulate('onEdit', mock Event); doesn't work

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.

3 participants