Feature/replace ava nyc enzyme and sinon for jest by Kelvur · Pull Request #8 · k14v/react-i18njs

Kelvur · 2020-03-28T19:54:56Z

No description provided.

rubeniskov

This kind of changes requires to be aware if the the improvement of testing legibility and config simplification is significant

So it would be great if the testing files were not changed the .spec.js to .test.js it would be easier to see the different between changes, now all deleted files are not referenced to the new ones, so the tracking is harder to follow. I recommend analize each review in order to clean all unnecessary configuration and aim a better version of this propourse .

Also I would like to remark, all kind of PR must be related to an issue with the motivation and a detailed plan to onboard the issue. This would help to discuss, prepare and estimate the required steps to reach the goal.

rubeniskov · 2020-03-28T20:14:37Z

src/I18nContext.test.js

+    jest.spyOn(console, 'warn');
+    render(<Consumer />);
+    expect(console.warn).toBeCalledTimes(1);
+    console.warn.mockRestore();


this would be more precise if the test checks the content of the message

Rigth, I will make the change

rubeniskov · 2020-03-29T02:31:21Z

src/I18nProvider.test.js

+    const { getByText, rerender } = render(<I18nProvider i18n={i18n}><Consumer word="Teléfono" /></I18nProvider>);
+    expect(getByText('Phone')).toBeInTheDocument();
+    await i18n.setLocale('de');
+    rerender(<I18nProvider i18n={i18n}><Consumer word="Teléfono" /></I18nProvider>);


Executing the tests, I receive this error

Yeah, that error is resolved in a future PR

rubeniskov · 2020-03-29T02:34:47Z

src/setupTests.ts

@@ -0,0 +1,5 @@
+// jest-dom adds custom jest matchers for asserting on DOM nodes.


This file seems that came from a boilerplate configuration but I don't know if is required for this feature

The extension is wrong but the file is neccessary to add the testing/library-react assertings to jest

rubeniskov · 2020-03-29T02:42:38Z

src/testUtils.js

+    const i18n = i18njs(I18N_DATA);
+    i18n.on('loaded', () => resolve(i18n));
+  });
+}


is there any miss configuration about eslinter?, I notice this semicollon is not revealing through the terminal output when the linter executes

I don't change the eslint config, but I had thought of change it in the future

rubeniskov · 2020-03-29T15:32:51Z

babel.config.js

@@ -0,0 +1,41 @@
+


This feature seems to try to resolve a conditional environment setting,
To preserve the project settings as much as possible and keep it centralized, I suggest follow the previous method using a Babel feature to solve this problem.

https://babeljs.io/docs/en/6.26.3/babelrc#env-option

Also recommend not include this commit to keep the tree changes clean and simple (rebase from the begining of this PR)

I don't find any way to do this with package.json, can you show me a example?

All the configuration available in .babelrc are compatible inside the package.sjon babel declaration.

https://babeljs.io/docs/en/6.26.3/babelrc#use-via-packagejson

{ "babel": { "env": { // Specific configuration for each enviroment "test": { .... } } } }

rubeniskov · 2020-03-29T15:47:13Z

src/useTranslation.test.js

+  });
+
+  it('translates correctly', async () => {
+    const i18n = await fetchI18n();


Same as above, where is the assertion to check the translation is the expected?

rubeniskov · 2020-03-29T15:49:46Z

src/withI18n.test.js

+    render(<I18nProvider i18n={i18n}><WrappedComponent /></I18nProvider>);
+  });
+
+  // it('translates correctly', async () => {


Commented specs? use .skip if the test is planned to be tested in a future or remove if not need it

Rigth, the test fail actually, I thought of removing the comments when the code is fixed

rubeniskov · 2020-03-29T15:52:01Z

package.json

+    ],
+    "testEnvironment": "jest-environment-jsdom-fourteen",
+    "transform": {
+      "^.+\\.(js|jsx)$": "<rootDir>/node_modules/babel-jest"


In this case the project is not and react app so there's no other kind of files rather than js

The idea was to have the same config in all the projects, not just the bare minimun

rubeniskov · 2020-03-29T15:55:20Z

package.json

+    "roots": [
+      "<rootDir>/src"
+    ],
+    "collectCoverageFrom": [


This is redudant theres no other format inside src directory

rubeniskov · 2020-03-29T15:56:04Z

package.json

+    "collectCoverageFrom": [
+      "src/**/*.{js,jsx}"
+    ],
+    "setupFilesAfterEnv": [


This file has a previous review to see if is really need it

Kelvur added 6 commits March 28, 2020 20:48

chore: replace actual testing packages by jest

339c43b

chore: remove previous test config

bf97c4f

chore: add jest config

5a47223

test: remove previous test

cfc17f0

test: rewrite the previous test in a jest/testing-library form

a00f863

fix: git push hook

3b70703

Kelvur requested a review from rubeniskov March 28, 2020 19:54

rubeniskov reviewed Mar 29, 2020

View reviewed changes

Kelvur added 3 commits March 30, 2020 17:38

test: add missing test

5509a8a

test: uncomment the test in withI18n and skip it

c391ac7

fix: rename setupTest.ts to .js

0a54fd5

		@@ -0,0 +1,5 @@
		// jest-dom adds custom jest matchers for asserting on DOM nodes.

Conversation

Kelvur commented Mar 28, 2020

Uh oh!

rubeniskov left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rubeniskov Mar 31, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

rubeniskov left a comment •

edited

Loading

rubeniskov Mar 31, 2020 •

edited

Loading