Fix Bug 1513203, add Jest and True, and initial tests #5162

schalkneethling · 2018-12-11T12:36:16Z

@davidflanagan As mentioned in the bug, this adds two frameworks:

Jest
True

For True I had to also add Mocha as the test runner. I am looking into writing a little bridge so I can also run True with Jest, so that is temporary. I also added a small number of tests for each.

After this is merged, I will be pushing a lot more tests for both. r?

schalkneethling · 2018-12-11T15:02:07Z

Looks like someone has already gotten Jest to work with True[https://github.com/oddbird/true/issues/132], looking into it. 🎉

davidflanagan

Thanks for asking me to review this Schalk. I learned a lot about Jest, True, and our own codebase.

I've left a number of suggestions, most of which are nits and not blocking.

But I am requesting changes because I think that the new utils file you create here doesn't appear to get bundled into build/js/payments.js where it is used. Maybe I just don't understand how we bundle the code, but I'm worried that this will break the payments flow as it stands.

davidflanagan · 2018-12-11T18:15:09Z

.eslintrc.js

+    'parserOptions': {
+        'ecmaVersion': 6
+    },
+    "plugins": ["jest"],


You're enabling the plugin, but not adding anything to rules or extends... Or does the plugin enable some of its lint rules by default? Or maybe this is just so that you can have the jest/globals in env above.

Should we go ahead and enable the recommended set of jest linting rules to impose some kind of testing discipline? Or will that just be more trouble than it is worth?

Or does the plugin enable some of its lint rules by default? Or maybe this is just so that you can have the jest/globals in env above.

It adds some of its own rules, but also adds the relevant globals so eslint does not complain about undefined globals such as describe and it

davidflanagan · 2018-12-11T18:23:04Z

package.json

+        "optim:svg": "svgo jinja2/includes/icons/* --disable={removeViewBox,removeTitle} --multipass --pretty",
+        "precommit": "npm run eslint && npm run stylelint",
+        "test:js": "jest",
+        "test:sass": "mocha __sass_tests__/test-sass.js"


I'm not a fan of all the underscores in __sass_tests__. Can we put the tests in tests/styles/ or in static/styles/tests instead?

Separately, what's the deal with the colons in the script names? Does doing this way mean I can type npm run test to have it run both test:js and test:sass? If so, that's great. Or if this is a package.json convention that I'm just not aware of, then it is totally fine by me. But if you're just making up names here, I'd suggest a hyphen instead of the colons.

The colons is a convention. Just running npm test will not run them all but, you can add npm-run-all and then define test to run both. This just enable me to separate out the two sets so, you can either run JS tests or SaSS tests.

In general this is still evolving, this PR being the first step in getting the base in place.

This has been cleaned up. Now we only use Jest 🎉

davidflanagan · 2018-12-11T18:26:31Z

package.json

+    "dependencies": {
+        "clean-css-cli": "4.2.1",
+        "eslint": "5.1.0",
+        "eslint-plugin-jest": "22.1.2",


I don't understand why this is a dependency and not a devDependency. Actually, all of these dependencies look like developer dependencies. What am I missing?

Ah. I bet that dependencies are things that we need to run inside the container. So anything involved in the make build-static, for example, would have to be here rather than in devDependencies. So I get why clean-css and uglify-js and sass are here. But I'm still surprised that the linters are here.

You are 100% correct. I believe we also run eslint on CI so, adding the Jest plugin ensures eslint will not break because of Jest globals.

davidflanagan · 2018-12-11T18:37:26Z

kuma/settings/common.py

-            'js/payments-faq.js',
-            'js/payments-confirmation.js',
+            'js/components/payments/payments-faq.js',
+            'js/components/payments/payments-confirmation.js',


I was not expecting to see our JS packaging configuration in python config file! I'm learning a lot by reviewing this patch.

This moving the files around seems like it could have been a different PR, but it seems like a reasonable thing to do.

Moving it out of Python and into a Nodejs based tool chain is part of the work I/we need to do ;) I could totally have moved this is a separate PR, but as I was touching these, I thought I might as well start moving things into a more "component" based file structure at least.

Also, having tests will aid a great deal in the next steps with regards to the toolchain, as well as removing dependencies such as jQuery etc.

davidflanagan · 2018-12-11T18:47:21Z

jest.config.js

+
+    // The glob patterns Jest uses to detect test files
+    // testMatch: [
+    //   "**/__tests__/**/*.js?(x)",


I was hoping there was a config option like this one. Since our python tests are in tests/ directories, perhaps we can use that same convention for our js tests and avoid the ugly underscores!

Good point. I was following the Jest convention over configuration path, but we surely can abandon that in favor of a probably cleaner folder structure.

davidflanagan · 2018-12-11T19:52:41Z

kuma/static/js/components/payments/payments-handler-utils.js

@@ -0,0 +1,23 @@
+var mdn = window.mdn || {};
+var paymentsHandlerUtils = {
+    getNewValue: function(selectedAmount) {


The name getNewValue is not descriptive enough for a utility function. Maybe something like addCurrencyPrefix()? And then add a comment explaining why it returns the empty string instead of '$0'?

Yup, pushed to soon ;p

davidflanagan · 2018-12-11T19:54:58Z

kuma/static/js/components/payments/payments-handler-utils.js

+var paymentsHandlerUtils = {
+    getNewValue: function(selectedAmount) {
+        'use strict';
+        return selectedAmount < 1 || isNaN(selectedAmount)


It seems strange to me that this code compares the input to a number before testing to see if it is a number. I'd reverse the sides of the ||. And add a comment somewhere saying that the function expects a string argument and that it depends on JS's loose string-to-number conversions.

davidflanagan · 2018-12-11T19:56:12Z

kuma/static/js/components/payments/payments-handler-utils.js

+            ? ''
+            : '$' + selectedAmount;
+    },
+    getSelectedAmount: function(value) {


Maybe call this roundSelectedAmount() to more closely indicate what it actually does?

davidflanagan · 2018-12-11T19:58:11Z

kuma/static/js/components/payments/payments-handler-utils.js

+    },
+    getSelectedAmount: function(value) {
+        'use strict';
+        return value % 1 === 0 ? parseInt(value, 10) : parseFloat(value).toFixed(2);


Worth commenting that the input is usually a string and that the function depends on the modulo operator working with the string version of numbers.

Also: it really bothers me that this function sometimes returns a number and sometimes returns a string. If we ever start using flow, this will cause all kinds of trouble. Would it be okay to use String(parseInt(value,10)) instead of just returning the result of parseInt()?

Also bothers me ;) I do not believe any math is done on this so, casting it to a String certainly makes sense.

It's a pitty that this was not documented when it was written. Currently it seems to me the only real reason parseInt is being used, is to convert something like 2.0 to 2.

2.0 % 1 === 0 ? parseInt(2.0, 10) : parseFloat(2.0).toFixed(2); // returns 2

Anything else like say 2.01, will go to the parseFloat side and be returned as a String

2.01 % 1 === 0 ? parseInt(2.0, 10) : parseFloat(2.0).toFixed(2); // returns "2.01"

I guess what I am saying is, why not just parse all values to a float and return a String? @jwhitlock Do you know why this is?

/me sure I am missing something obvious.

I assume the reason not to use just the parseFloat() route is that then you'd end up with "$2.00" instead of "$2". But something like `parseFloat(value).toFixed(2).replace(/.00$/, '') might do the same thing in an arguable cleaner way.

Yup. This was a decision made that I was not aware of so, I am not going to change it for now. Will put a pin in it, and cycle back later.

davidflanagan · 2018-12-11T20:03:24Z

kuma/settings/common.py

@@ -959,17 +959,17 @@ def pipeline_one_scss(slug, **kwargs):
    },
    'payments': {
        'source_filenames': (
-            'js/payments-handler.js',
-            'js/payments-faq.js',
+            'js/components/payments/payments-handler.js',


Don't you need to add the new payments-handler-utils.js file here?

Or maybe I just don't understand our js packaging scheme.

Nope, nope, you are correct. Doh!

schalkneethling · 2018-12-12T08:19:07Z

Thanks for the detailed review @davidflanagan - addressing your comments.

schalkneethling · 2018-12-12T09:19:53Z

@davidflanagan Addressed all comments, also removed Mocha, so we are now only using Jest. When I say all comments, I mean all but #5162 (comment), which warrants more discussion.

davidflanagan

It is great to see Mocha removed. You've fixed the bundling issue, which was the only blocking change I'd identified. Everything else was just nits and suggestions, so this LGTM!

schalkneethling · 2018-12-13T06:14:58Z

Thanks @davidflanagan 🌮

Fix Bug 1513203, add Jest and True, and initial tests

676a087

schalkneethling added the comp: frontend Component: Frontend label Dec 11, 2018

schalkneethling requested a review from davidflanagan December 11, 2018 12:36

davidflanagan suggested changes Dec 11, 2018

View reviewed changes

Remove mocha, use only Jest, address review comments

4391b14

davidflanagan approved these changes Dec 12, 2018

View reviewed changes

schalkneethling merged commit d3e69b1 into mdn:master Dec 13, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Bug 1513203, add Jest and True, and initial tests #5162

Fix Bug 1513203, add Jest and True, and initial tests #5162

schalkneethling commented Dec 11, 2018

schalkneethling commented Dec 11, 2018

davidflanagan left a comment

davidflanagan Dec 11, 2018

schalkneethling Dec 12, 2018

davidflanagan Dec 11, 2018

schalkneethling Dec 12, 2018

schalkneethling Dec 12, 2018

davidflanagan Dec 11, 2018

davidflanagan Dec 11, 2018

schalkneethling Dec 12, 2018

davidflanagan Dec 11, 2018

schalkneethling Dec 12, 2018

schalkneethling Dec 12, 2018

davidflanagan Dec 11, 2018

schalkneethling Dec 12, 2018

davidflanagan Dec 11, 2018

schalkneethling Dec 12, 2018

davidflanagan Dec 11, 2018

davidflanagan Dec 11, 2018

davidflanagan Dec 11, 2018

schalkneethling Dec 12, 2018

schalkneethling Dec 12, 2018

davidflanagan Dec 12, 2018

schalkneethling Dec 13, 2018

davidflanagan Dec 11, 2018

davidflanagan Dec 11, 2018

schalkneethling Dec 12, 2018

schalkneethling commented Dec 12, 2018

schalkneethling commented Dec 12, 2018

davidflanagan left a comment

schalkneethling commented Dec 13, 2018

Fix Bug 1513203, add Jest and True, and initial tests #5162

Fix Bug 1513203, add Jest and True, and initial tests #5162

Conversation

schalkneethling commented Dec 11, 2018

schalkneethling commented Dec 11, 2018

davidflanagan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

schalkneethling commented Dec 12, 2018

schalkneethling commented Dec 12, 2018

davidflanagan left a comment

Choose a reason for hiding this comment

schalkneethling commented Dec 13, 2018