Skip to content
This repository was archived by the owner on Dec 5, 2019. It is now read-only.

test: add && backport tests from webpack core #56

Merged
merged 4 commits into from
Jun 29, 2017

Conversation

hulkish
Copy link
Contributor

@hulkish hulkish commented Jun 24, 2017

Closes #1. This is a port of the existing UglifyJsPlugin tests.

I've converted them to jest format and also added a snapshot test. I only added one snapshot test for now.

@codecov
Copy link

codecov bot commented Jun 24, 2017

Codecov Report

❗ No coverage uploaded for pull request base (master@0f58cd0). Click here to learn what that means.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #56   +/-   ##
=========================================
  Coverage          ?   96.95%           
=========================================
  Files             ?        2           
  Lines             ?      164           
  Branches          ?       49           
=========================================
  Hits              ?      159           
  Misses            ?        5           
  Partials          ?        0
Impacted Files Coverage Δ
__tests__/helpers.js 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0f58cd0...735bf1d. Read the comment docs.

@michael-ciniawsky michael-ciniawsky changed the title Ported tests from core webpack repo & added tests test: add && backport tests from webpack core Jun 25, 2017
Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

Thx :) so far

const MemoryFileSystem = require("memory-fs");
const webpack = require("webpack");

exports.PluginEnvironment = class PluginEnvironment {
Copy link
Member

Choose a reason for hiding this comment

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

What is this for ? 🙃

Copy link
Contributor Author

@hulkish hulkish Jun 25, 2017

Choose a reason for hiding this comment

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

from here

The memory-fs is being used to run a webpack compile without needing to emit files for the test.

Copy link
Member

Choose a reason for hiding this comment

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

👍 Can we make this more generic and could you elaborate a bit more in detail about it, I would like to write docs about it then, for the webpack-defaults test boilerplate

ExpectedPlugin TestPlugin JestPlugin?

Copy link
Member

@michael-ciniawsky michael-ciniawsky Jun 26, 2017

Choose a reason for hiding this comment

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

Would it also be possible to get the MemoryFS output 'bundle' and snapshot test it?

test('${name}', () => {
   compiler(fixture, config)
     .then((result) => {
        result.stats
        result.output // MemoryFS Instance as `bundle.js`
        expect(result.output).toMatchSnapshot()
      })
     .catch((err) => err)
})

Copy link
Contributor Author

@hulkish hulkish Jun 26, 2017

Choose a reason for hiding this comment

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

Yes, and that's probably better anyway. I'll add that in.

Copy link
Member

@michael-ciniawsky michael-ciniawsky Jun 26, 2017

Choose a reason for hiding this comment

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

How does result.output look like in (terms of noise), could you post it here when ready ? 😛
If we agree on 'extending' a base config we could evetually add CommonChunksPluginto the base to split the webpack runtime by default

[ 
  new CommonChunksPlugin({ 
    name: [ 'runtime', `${fixture}` ], 
    minChunks: Infinity 
  }) 
].concat(config.plugins || [])
webpackJsonp([`${fixture}`],{...bundle}

@@ -0,0 +1,17 @@
codecov:
Copy link
Member

Choose a reason for hiding this comment

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

We may need this in webpack-defaults instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wouldn't webpack-defaults just create this file here anyway? Or, do u mean it also needs to be added there?

Copy link
Member

Choose a reason for hiding this comment

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

yep, if we need codecov.yml for coverage to work correctly, we should add it to webpack-defaults aswell to keep repos in sync

package.json Outdated
],
"moduleDirectories": [
"node_modules"
"coverageDirectory": "./coverage/",
Copy link
Member

Choose a reason for hiding this comment

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

Are most of these not defaults anyways? Also the question if we should add this to webpack-defaults then

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added coverageDirectory to try and get the codecov ci check to succeed. Right now my best guess is that it's only not succeeding because it compares the codecov to master branch - which currently has no tests. Please advise.

};


exports.runCompiler = function runCompiler(compiler) {
Copy link
Member

@michael-ciniawsky michael-ciniawsky Jun 25, 2017

Choose a reason for hiding this comment

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

Why not function compile (fixture, config) {} [Discuss], so

import { compile } from './helpers/webpack.js'
import UgilfyJSPlugin from '..'

test('${name}', () => {
   const config = { plugins: [ new UgilfyJSPlugin() ] }

   compile(`${fixture}`, config).then((result) => {
      // ...
   }) 
})

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, I'm ok with either direction. I hadn't added many snapshots tests yet - but I built this with the idea of making many in mind and thought it would be useful.

Copy link
Member

Choose a reason for hiding this comment

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

This is enterily open for discussion and maybe out of scope of this PR, the direction is fine and changing it in a following PR is totally ok

@@ -0,0 +1,824 @@
"use strict";
Copy link
Member

Choose a reason for hiding this comment

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

Possible to split these into separate files ? e.g

|– test
| |– __snapshots__
| |– index.test.js  (Basic, with defaults)
| |– options1.test.js
| |– options2.test.js
| |– .... 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, but can you clarify? Do u mean put snapshot tests in diff file? Or have the snapshots themselves be separated?

Copy link
Member

Choose a reason for hiding this comment

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

The __snapshots__ directory is just in there as a 'where are we' orientation point, jest will create a separate snapshot for each ${name}.test.js in __snapshots__. The important part is to split test into logical groups, so particular parts can be tested more independently

Copy link
Member

Choose a reason for hiding this comment

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

You could look at webpack-cli to see how we layered our tests :)

package.json Outdated
"babel-plugin-syntax-object-rest-spread": "^6.13.0",
"babel-plugin-transform-object-rest-spread": "^6.20.2",
"babel-preset-es2015": "^6.18.0",
"eslint": "^3.13.1",
"eslint-config-airbnb": "^14.0.0",
"eslint-plugin-import": "^2.2.0",
"eslint-plugin-jest": "^20.0.3",
Copy link
Member

Choose a reason for hiding this comment

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

Needed ? Also needs to be in snyc with webpack-defaults then :)

Copy link
Member

Choose a reason for hiding this comment

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

I don't fully know if this is needed, as most of these are done during dev and caught up during PR reviews/submits:
skjermbilde 2017-06-25 kl 19 52 01

Copy link
Contributor Author

@hulkish hulkish Jun 25, 2017

Choose a reason for hiding this comment

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

One issue I can into is that jest needs to be a global. I can remove this plugin and just allow jest to be a global, I suppose.

Update: Actually, I just checked and there's more than just jest that needs to be global. For example: expect/test/it etc. IF you know another way that removes the need for this plugin, let me know.

I suppose turning on env.jasmine = true might solve this... but might not exactly line up with jest.

Also, this seems related - lemme know how we wanna go about this.

Copy link
Member

@michael-ciniawsky michael-ciniawsky Jun 25, 2017

Choose a reason for hiding this comment

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

kk I can't really say tbh, bc I don't use it myself and as a StandardJS user I never really dealt with ESLint aswell :), if ESLint complains it needs t be solved either in eslint-config-webpack or via a PR adding this plugin to webpack-defaultsin the long-term

@michael-ciniawsky michael-ciniawsky added this to the 0.4.7 milestone Jun 25, 2017
package.json Outdated
],
"moduleDirectories": [
"node_modules"
"coverageDirectory": "./coverage/",
Copy link
Member

Choose a reason for hiding this comment

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

coverageDirectory defaults to coverage

@hulkish
Copy link
Contributor Author

hulkish commented Jun 27, 2017

@michael-ciniawsky better?

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Excellent!

Copy link
Member

@michael-ciniawsky michael-ciniawsky left a comment

Choose a reason for hiding this comment

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

Thx 👍

const createCompiler = require("./helpers").createCompiler;
const compile = require("./helpers").compile;

describe("snapshotss", () => {
Copy link
Member

Choose a reason for hiding this comment

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

snapshotss => snapshots

@hulkish
Copy link
Contributor Author

hulkish commented Jun 27, 2017

also added webpack-contrib/webpack-defaults#77 for codecov.yml

@michael-ciniawsky
Copy link
Member

michael-ciniawsky commented Jun 27, 2017

travis.yml needs an update for node

node_js:
- - "4"
- - "5"
- - "6"
+ - node (Current)
+ - 6 (LTS)
+ - 4 (Maintenance)

invalid-options.test.js is failing due to path mismatches in the Error StackTrace, the individual part could normally be replaced with a regex .*\/... but this one is hairy to substitute 😛

@@ -15,8 +16,11 @@ describe("when applied with invalid options", () => {
}).apply(compiler);

return compile(compiler).then((stats) => {
expect(stats.compilation.warnings).toMatchSnapshot("warnings");
expect(stats.compilation.errors).toMatchSnapshot("errors");
const errors = stats.compilation.errors.map(cleanErrorStack);
Copy link
Member

Choose a reason for hiding this comment

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

clever :)

@hulkish
Copy link
Contributor Author

hulkish commented Jun 28, 2017

@michael-ciniawsky interesting... it fails in ci on node 4 only: https://travis-ci.org/webpack-contrib/uglifyjs-webpack-plugin/jobs/247753398#L242

maybe i remove stack and only render the error message? since node 4 captures error stack differently?

@hulkish hulkish changed the base branch from master to feature-defaults June 28, 2017 03:22
hulkish added 2 commits June 28, 2017 01:37
added eslint-plugin-jest for tests, upgraded jest, babel-jest & webpack dev deps

cleanup, corrected snapshot test

fixed linting errors

added eslintignore for test stubs

corrected eslintignore paths

added codecov config

fixing codecov

corrected jest test/src/coverage paths

removed coverageDirectory: './coverage/' from jest config since it's default

separated snapshot tests

made createCompiler options replaceable, improved default options test

separated tests by logic into separate test files

improved snapshot tests

cleaned up stacktraces in snapshot output, to allow snapshots to not fail just because paths change in error stack

fix typo

cleaned up test compilation error stacks to be more generic

more cleanup
@michael-ciniawsky michael-ciniawsky modified the milestones: 1.0.0, 0.4.7 Jun 28, 2017
@michael-ciniawsky
Copy link
Member

Can you remove the yarn.lock file please webpack-defaults uses package-lock.json&& npm@5 :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants