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

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

Merged
merged 2 commits into from Dec 13, 2018

Conversation

Projects
None yet
2 participants
@schalkneethling
Copy link
Collaborator

schalkneethling commented Dec 11, 2018

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

  1. Jest
  2. 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

This comment has been minimized.

Copy link
Collaborator Author

schalkneethling commented Dec 11, 2018

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

@davidflanagan
Copy link
Collaborator

davidflanagan left a comment

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.

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

This comment has been minimized.

@davidflanagan

davidflanagan Dec 11, 2018

Collaborator

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?

This comment has been minimized.

@schalkneethling

schalkneethling Dec 12, 2018

Author Collaborator

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

"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"

This comment has been minimized.

@davidflanagan

davidflanagan Dec 11, 2018

Collaborator

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.

This comment has been minimized.

@schalkneethling

schalkneethling Dec 12, 2018

Author Collaborator

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 comment has been minimized.

@schalkneethling

schalkneethling Dec 12, 2018

Author Collaborator

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

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

This comment has been minimized.

@davidflanagan

davidflanagan Dec 11, 2018

Collaborator

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?

This comment has been minimized.

@davidflanagan

davidflanagan Dec 11, 2018

Collaborator

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.

This comment has been minimized.

@schalkneethling

schalkneethling Dec 12, 2018

Author Collaborator

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.

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

This comment has been minimized.

@davidflanagan

davidflanagan Dec 11, 2018

Collaborator

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.

This comment has been minimized.

@schalkneethling

schalkneethling Dec 12, 2018

Author Collaborator

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.

This comment has been minimized.

@schalkneethling

schalkneethling Dec 12, 2018

Author Collaborator

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.


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

This comment has been minimized.

@davidflanagan

davidflanagan Dec 11, 2018

Collaborator

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!

This comment has been minimized.

@schalkneethling

schalkneethling Dec 12, 2018

Author Collaborator

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.

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

This comment has been minimized.

@davidflanagan

davidflanagan Dec 11, 2018

Collaborator

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'?

This comment has been minimized.

@schalkneethling

schalkneethling Dec 12, 2018

Author Collaborator

Yup, pushed to soon ;p

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

This comment has been minimized.

@davidflanagan

davidflanagan Dec 11, 2018

Collaborator

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.

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

This comment has been minimized.

@davidflanagan

davidflanagan Dec 11, 2018

Collaborator

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

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

This comment has been minimized.

@davidflanagan

davidflanagan Dec 11, 2018

Collaborator

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()?

This comment has been minimized.

@schalkneethling

schalkneethling Dec 12, 2018

Author Collaborator

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

This comment has been minimized.

@schalkneethling

schalkneethling Dec 12, 2018

Author Collaborator

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.

This comment has been minimized.

@davidflanagan

davidflanagan Dec 12, 2018

Collaborator

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.

This comment has been minimized.

@schalkneethling

schalkneethling Dec 13, 2018

Author Collaborator

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.

@@ -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',

This comment has been minimized.

@davidflanagan

davidflanagan Dec 11, 2018

Collaborator

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

This comment has been minimized.

@davidflanagan

davidflanagan Dec 11, 2018

Collaborator

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

This comment has been minimized.

@schalkneethling

schalkneethling Dec 12, 2018

Author Collaborator

Nope, nope, you are correct. Doh!

@schalkneethling

This comment has been minimized.

Copy link
Collaborator Author

schalkneethling commented Dec 12, 2018

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

@schalkneethling

This comment has been minimized.

Copy link
Collaborator Author

schalkneethling commented Dec 12, 2018

@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
Copy link
Collaborator

davidflanagan left a comment

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

This comment has been minimized.

Copy link
Collaborator Author

schalkneethling commented Dec 13, 2018

Thanks @davidflanagan 🌮

@schalkneethling schalkneethling merged commit d3e69b1 into mozilla:master Dec 13, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
security/snyk - package.json (mdn) No new, high severity issues
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment