Skip to content
This repository has been archived by the owner on Aug 26, 2022. It is now read-only.

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

Merged
merged 2 commits into from Dec 13, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 8 additions & 1 deletion .eslintrc.js
@@ -1,8 +1,15 @@
module.exports = {
'env': {
'browser': true,
'jquery': true
'es6': true,
"jest/globals": true,
'jquery': true,
"node": true
},
'parserOptions': {
'ecmaVersion': 6
},
"plugins": ["jest"],

Choose a reason for hiding this comment

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

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?

Copy link
Author

Choose a reason for hiding this comment

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

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

'extends': 'eslint:recommended',
'rules': {
'no-global-assign': 'error',
Expand Down
27 changes: 27 additions & 0 deletions __tests__/payments-handler-utils.test.js
@@ -0,0 +1,27 @@
const paymentsHandlerUtils = require('../kuma/static/js/components/payments/payments-handler-utils.js');

Choose a reason for hiding this comment

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

Could we have this test live in static/js/components/payments/tests/ so that the tests are near the files they're testing?


describe('addCurrencyPrefix', function() {
it('returns an empty string for non numerals and values less that 1', function() {
expect(paymentsHandlerUtils.addCurrencyPrefix(0.5)).toEqual('');
expect(paymentsHandlerUtils.addCurrencyPrefix('ten')).toEqual('');
});

it('returns a dollar amount as a string for values greater than 1', function() {
expect(paymentsHandlerUtils.addCurrencyPrefix(1)).toEqual('$1');
expect(paymentsHandlerUtils.addCurrencyPrefix('10')).toEqual('$10');
});
});

describe('getSelectedAmount', function() {
it('returns an integer for whole numbers', function() {
expect(paymentsHandlerUtils.getSelectedAmount(1)).toEqual(1);
expect(paymentsHandlerUtils.getSelectedAmount('2')).toEqual(2);
expect(typeof 10).toEqual(typeof paymentsHandlerUtils.getSelectedAmount('10'));
});

it('returns a floating point number as a string, limited to two decimal places for decimal values', function() {
expect(paymentsHandlerUtils.getSelectedAmount(1.2345)).toEqual('1.23');

Choose a reason for hiding this comment

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

Maybe add a test for the rounding behavior? Does 1.2354 round down to 1.23 or up to 1.24?

expect(paymentsHandlerUtils.getSelectedAmount(1.2)).toEqual('1.20');
expect(paymentsHandlerUtils.getSelectedAmount('10.9')).toEqual('10.90');
});
});
37 changes: 37 additions & 0 deletions __tests__/sass/mdn/_typography-mixin-test.scss
@@ -0,0 +1,37 @@
@include describe('-remify') {
@include it('Converts pixel based units to rem units') {
$expect: .88889rem; /* stylelint-disable number-leading-zero */

/* Docs: http://oddbird.net/true/docs/api-assert-values.html#mixin--assert-equal */
@include assert-equal(-remify(16px), $expect, null, true);
}
}

@include describe('set-font-size') {
@include it(
'Returns a rem based font-size declaration, and a pixel based fallback'
) {
@include assert {
@include output {
@include set-font-size(16px);
}

@include expect {
font-size: 16px;
font-size: 0.88889rem;
}
}
}

@include it('Returns only a rem based font-size declaration') {
@include assert {
@include output {
@include set-font-size(2rem);
}

@include expect {
font-size: 2rem;
}
}
}
}
5 changes: 5 additions & 0 deletions __tests__/sass/test-sass.js
@@ -0,0 +1,5 @@
var path = require('path');
var sassTrue = require('sass-true');

var sassFile = path.join(__dirname, 'test.scss');
sassTrue.runSass({file: sassFile}, describe, it);
7 changes: 7 additions & 0 deletions __tests__/sass/test.scss
@@ -0,0 +1,7 @@
/* Requirements */
@import '../../node_modules/sass-true/sass/true';
@import '../../kuma/static/styles/includes/vars';
@import '../../kuma/static/styles/includes/mixins';

/* Tests */
@import 'mdn/typography-mixin-test';
180 changes: 180 additions & 0 deletions jest.config.js
@@ -0,0 +1,180 @@
// For a detailed explanation regarding each configuration property, visit:
// https://jestjs.io/docs/en/configuration.html

module.exports = {
// All imported modules in your tests should be mocked automatically
// automock: false,

// Stop running tests after the first failure
// bail: false,

// Respect "browser" field in package.json when resolving modules
// browser: false,

// The directory where Jest should store its cached dependency information
// cacheDirectory: "/var/folders/l1/mjqxjq7j08jb6z6_f7j5wcgw0000gn/T/jest_dx",

// Automatically clear mock calls and instances between every test
clearMocks: true,

// Indicates whether the coverage information should be collected while executing the test
// collectCoverage: false,

// An array of glob patterns indicating a set of files for which coverage information should be collected
// collectCoverageFrom: null,

// The directory where Jest should output its coverage files
coverageDirectory: 'coverage'

// An array of regexp pattern strings used to skip coverage collection
// coveragePathIgnorePatterns: [
// "/node_modules/"
// ],

// A list of reporter names that Jest uses when writing coverage reports
// coverageReporters: [
// "json",
// "text",
// "lcov",
// "clover"
// ],

// An object that configures minimum threshold enforcement for coverage results
// coverageThreshold: null,

// Make calling deprecated APIs throw helpful error messages
// errorOnDeprecated: false,

// Force coverage collection from ignored files usin a array of glob patterns
// forceCoverageMatch: [],

// A path to a module which exports an async function that is triggered once before all test suites
// globalSetup: null,

// A path to a module which exports an async function that is triggered once after all test suites
// globalTeardown: null,

// A set of global variables that need to be available in all test environments
// globals: {},

// An array of directory names to be searched recursively up from the requiring module's location
// moduleDirectories: [
// "node_modules"
// ],

// An array of file extensions your modules use
// moduleFileExtensions: [
// "js",
// "json",
// "jsx",
// "node"
// ],

// A map from regular expressions to module names that allow to stub out resources with a single module
// moduleNameMapper: {},

// An array of regexp pattern strings, matched against all module paths before considered 'visible' to the module loader
// modulePathIgnorePatterns: [],

// Activates notifications for test results
// notify: false,

// An enum that specifies notification mode. Requires { notify: true }
// notifyMode: "always",

// A preset that is used as a base for Jest's configuration
// preset: null,

// Run tests from one or more projects
// projects: null,

// Use this configuration option to add custom reporters to Jest
// reporters: undefined,

// Automatically reset mock state between every test
// resetMocks: false,

// Reset the module registry before running each individual test
// resetModules: false,

// A path to a custom resolver
// resolver: null,

// Automatically restore mock state between every test
// restoreMocks: false,

// The root directory that Jest should scan for tests and modules within
// rootDir: null,

// A list of paths to directories that Jest should use to search for files in
// roots: [
// "<rootDir>"
// ],

// Allows you to use a custom runner instead of Jest's default test runner
// runner: "jest-runner",

// The paths to modules that run some code to configure or set up the testing environment before each test
// setupFiles: [],

// The path to a module that runs some code to configure or set up the testing framework before each test
// setupTestFrameworkScriptFile: null,

// A list of paths to snapshot serializer modules Jest should use for snapshot testing
// snapshotSerializers: [],

// The test environment that will be used for testing
// testEnvironment: "jest-environment-jsdom",

// Options that will be passed to the testEnvironment
// testEnvironmentOptions: {},

// Adds a location field to test results
// testLocationInResults: false,

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

Choose a reason for hiding this comment

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

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!

Copy link
Author

Choose a reason for hiding this comment

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

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.

// "**/?(*.)+(spec|test).js?(x)"
// ],

// An array of regexp pattern strings that are matched against all test paths, matched tests are skipped
// testPathIgnorePatterns: [
// "/node_modules/"
// ],

// The regexp pattern Jest uses to detect test files
// testRegex: "",

// This option allows the use of a custom results processor
// testResultsProcessor: null,

// This option allows use of a custom test runner
// testRunner: "jasmine2",

// This option sets the URL for the jsdom environment. It is reflected in properties such as location.href
// testURL: "http://localhost",

// Setting this value to "fake" allows the use of fake timers for functions such as "setTimeout"
// timers: "real",

Choose a reason for hiding this comment

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

I find fake timers are often useful. Don't know why Jest would disable that feature by default... Consider setting this to fake?


// A map from regular expressions to paths to transformers
// transform: null,

// An array of regexp pattern strings that are matched against all source file paths, matched files will skip transformation
// transformIgnorePatterns: [
// "/node_modules/"
// ],

// An array of regexp pattern strings that are matched against all modules before the module loader will automatically return a mock for them
// unmockedModulePathPatterns: undefined,

// Indicates whether each individual test should be reported during the run
// verbose: null,

// An array of regexp patterns that are matched against all source file paths before re-running tests in watch mode
// watchPathIgnorePatterns: [],

// Whether to use watchman for file crawling
// watchman: true,
};
11 changes: 6 additions & 5 deletions kuma/settings/common.py
Expand Up @@ -959,17 +959,18 @@ def pipeline_one_scss(slug, **kwargs):
},
'payments': {
'source_filenames': (
'js/payments-handler.js',
'js/payments-faq.js',
'js/components/payments/payments-handler-utils.js',
'js/components/payments/payments-handler.js',

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Nope, nope, you are correct. Doh!

'js/components/payments/payments-faq.js',
'js/tooltip.js',
'js/payments-manage.js',
'js/components/payments/payments-manage.js',
),
'output_filename': 'build/js/payments.js',
},
'payments-confirmation': {
'source_filenames': (
'js/payments-faq.js',
'js/payments-confirmation.js',
'js/components/payments/payments-faq.js',
'js/components/payments/payments-confirmation.js',

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

),
'output_filename': 'build/js/payments-confirmation.js',
},
Expand Down
30 changes: 30 additions & 0 deletions kuma/static/js/components/payments/payments-handler-utils.js
@@ -0,0 +1,30 @@
var mdn = window.mdn || {};
var paymentsHandlerUtils = {
/**
* If the `selectedAmount` isNaN or less than 1, return and empty
* string else, prepend a dollar symbol and return.
* NOTE: This function relies on JavaScript's loose string-to-number conversions
* @param {String|Number} selectedAmount - The number to process
* @returns `selectedAmount` prefixed with a dollar symbol, or an empty string
*/
addCurrencyPrefix: function(selectedAmount) {
'use strict';
return isNaN(selectedAmount) || selectedAmount < 1
? ''
: '$' + selectedAmount;
},
getSelectedAmount: function(value) {

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

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.

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

}
};

/* First step towards using actual modules for our JS.
This will allow unit testing, and */
if (typeof exports === 'object') {
module.exports = paymentsHandlerUtils;
}

/* this will ensure it also works in the browser,
without yet needing Babel */
mdn.paymentsHandlerUtils = paymentsHandlerUtils;
Expand Up @@ -246,8 +246,8 @@
form.find('input[type=\'radio\']:checked').prop('checked', false);
}

selectedAmount = event.target.value % 1 === 0 ? parseInt(event.target.value) : parseFloat(event.target.value).toFixed(2);
var newValue = (selectedAmount < 1 || isNaN(selectedAmount)) ? '' : '$' + selectedAmount;
selectedAmount = mdn.paymentsHandlerUtils.getSelectedAmount(event.target.value);
var newValue = mdn.paymentsHandlerUtils.addCurrencyPrefix(selectedAmount);
amountToUpdate.html(newValue);

// Explicitly add `/month` on the payment button for the banner
Expand Down