Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve remaining build tests #1828

Merged
merged 1 commit into from
Dec 5, 2020
+640 −1,182
Merged

Improve remaining build tests #1828

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ node_modules
package-lock.json
create-snowpack-app/*/build
test/build/**/build
test/build/**/web_modules
test/create-snowpack-app/test-install
test/esinstall/**/web_modules
yarn-error.log
Expand Down
2 changes: 2 additions & 0 deletions CONTRIBUTING.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ yarn test:dev # might fail on windows, see #1171

### Snapshot tests

_Update Dec 2020: we’re working on improving this! Snapshots are now mostly gone from `test/build`, and we’ll be working through `test/esinstall` next. We‘ll wait to finish the work before updating this section, but know that this may become outdated soon._

The way our snapshot tests work is they test Snowpack by building the codebases in `test/build`. You'll almost always have a "failed" snapshot test when you make a contribution because your new change will make the final build different. You'll want to take a new snapshot. To do this run:

```bash
Expand Down
1 change: 0 additions & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@
"lint": "lerna run lint --parallel --scope=esinstall --scope=snowpack --scope=skypack",
"publish": "npm run build && lerna publish --no-private",
"format": "prettier --write '{snowpack,esinstall}/src/**/*.{ts,js}' '{test,plugins}/**/*.{ts,js}' '*.{js,json,md}' '**/*.{json,md}' '.github/**/*.{md,yml}' '!**/{_dist_,build,packages,pkg,TEST_BUILD_OUT,web_modules}/**' !test/create-snowpack-app/test-install",
"pretest": "node test/setup.js",
"test": "jest --testPathIgnorePatterns=/test-dev/ --test-timeout=30000",
"test:dev": "jest /test-dev/ --test-timeout=30000",
"test:docs": "cd www && yarn && yarn test --passWithNoTests"
Expand Down
14 changes: 11 additions & 3 deletions test/build/base-url-homepage/base-url-homepage.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,26 @@
const fs = require('fs');
const path = require('path');
const cheerio = require('cheerio');
const {setupBuildTest, readFiles} = require('../../test-utils');

const html = fs.readFileSync(path.join(__dirname, 'build', 'index.html'), 'utf8');
Copy link
Collaborator Author

@drwpow drwpow Dec 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

But with that setup script comes a caveat: none of these files exist when the test loads, so you‘ll have to read files in beforeAll(() => …

Copy link
Owner

Choose a reason for hiding this comment

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

I actually prefer this, touching globally shared test values is just asking for one test to modify them and then create unexpected behavior.

As we move to use the programatic interface & more "one fixture per test" tests, a separate files object would most likely be created for each test, so this is a good step in that direction.

const cwd = path.join(__dirname, 'build');

const $ = cheerio.load(html);
let files = {};

describe('packageManifest.homepage', () => {
beforeAll(() => {
setupBuildTest(__dirname);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This one line does have to be added to the beginning of every test, however, it can be omitted if a different setup is required. Hopefully it‘s the best of both worlds! And it keeps the functionality where if you only want to run a single test locally, only that one test will build.

Copy link
Owner

Choose a reason for hiding this comment

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

Yea I like this! However, using the synchronous execSync here probably slows these tests down a ton more than needed. To do this, Node/execa has to block the thread until the command continues. This prevents running multiple tests in parallel, unless Node is spinning up workers for each test file.

In either case, you can return a promise here, so strong +1 for using the async execa command and then awaiting that here, so that Jest can go do other stuff while the test folder sets up

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That‘s a good point about running in parallel. I usually try not to pre-emptively optimize something but it does make more sense to flow with how the tests are running. Will change


files = readFiles(['index.html'], {cwd});
});

it('baseUrl works for <link>', () => {
const $ = cheerio.load(files['/index.html']);
expect($('link[rel="icon"]').attr('href').startsWith('/static/')).toBe(true);
expect($('link[rel="stylesheet"]').attr('href').startsWith('/static/')).toBe(true);
});

it('baseUrl works for <script>', () => {
const $ = cheerio.load(files['/index.html']);
expect($('script').attr('src').startsWith('/static/')).toBe(true);
});
});
14 changes: 11 additions & 3 deletions test/build/base-url-remote/base-url-remote.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,20 @@
const fs = require('fs');
const path = require('path');
const cheerio = require('cheerio');
const {setupBuildTest, readFiles} = require('../../test-utils');

const html = fs.readFileSync(path.join(__dirname, 'build', 'index.html'), 'utf8');
const cwd = path.join(__dirname, 'build');

const $ = cheerio.load(html);
let files = {};
let $;

describe('buildOptions.baseUrl', () => {
beforeAll(() => {
setupBuildTest(__dirname);

files = readFiles(['index.html'], {cwd});
$ = cheerio.load(files['/index.html']);
});

it('baseUrl works for <link>', () => {
expect($('link[rel="icon"]').attr('href').startsWith('https://www.example.com/')).toBe(true);
expect($('link[rel="stylesheet"]').attr('href').startsWith('https://www.example.com/')).toBe(
Expand Down
27 changes: 18 additions & 9 deletions test/build/base-url/base-url.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,20 @@
const fs = require('fs');
const path = require('path');
const cheerio = require('cheerio');
const {setupBuildTest, readFiles} = require('../../test-utils');

const cwd = path.join(__dirname, 'build');

const html = fs.readFileSync(path.join(cwd, 'index.html'), 'utf8');
const indexJS = fs.readFileSync(path.join(cwd, 'index.js'), 'utf-8');
const distJS = fs.readFileSync(path.join(cwd, '_dist_', 'index.js'), 'utf-8');
const $ = cheerio.load(html);
let files = {};
let $;

describe('buildOptions.baseUrl', () => {
Copy link
Owner

Choose a reason for hiding this comment

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

also, top-level describe name should match the file name so that we keep the current behavior of yarn test base-url. Right now, we have the extra overhead of keeping track of two different names

Copy link
Collaborator Author

@drwpow drwpow Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I’m game for renaming the describe() names to be clearer in purpose, but I’m against renaming them to match filenames. No matter what they’re named, you still have to specify part of the filename with yarn test base-url. But they’re an opportunity to explain a test’s focus in more clear language, and not just repeat the folder name (Jest shows you both). But to that end, I’m not saying that they are human-readable clear language now and I’m fine with them changing; that’s just where I’d like them to be.

Copy link
Owner

Choose a reason for hiding this comment

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

Whoops, looks like I misunderstood. yarn test base-url seems to work in the existing main branch. I could have sworn I tried this and it didn't work. nvm!

beforeAll(() => {
setupBuildTest(__dirname);

files = readFiles(['index.html', 'index.js', '_dist_/index.js'], {cwd});
Copy link
Owner

Choose a reason for hiding this comment

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

also, comment as I'm writing my first test: do we need to maintain an allowlist like this? I'm seeing some unnecessary overhead managing this array and the different files[PATH] in the tests below. Since there's no connection between the two, I could see this getting out of date as well.

A better interface imo would be readFile(path), which looks up the file during the test itself:

it('...', (async () => {
  expect(await getFile('/index.js')).toEqual(...);
});

Copy link
Collaborator Author

@drwpow drwpow Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

You‘re right that the initialization is awkward. But reading file contents is the bulk of these tests, and most tests can‘t have an inline expect(await getFile(…)) like in your example. Most tests reuse file contents across tests, so it needs to exist in some shared variable.

In writing tests I found myself even getting confused by having to name every file in a project (how do I name /index.js vs /_dist_/index.js vs /_dist_/components/foo2.js?). I found it much easier to just refer to the filepath itself, both in reading & writing tests (files['/index.html'] is clearer than indexHTML). I think even if you scope variables within tests, you still have the same problem of having to come up with names for all these files, and even though it‘s a minor, optional, thing, I still find it helpful to establish a good pattern for the most common part of the build tests.

Re-reading files isn‘t really a big deal; I’m mostly concerned about the DX.

But that said, WDYT about a glob pattern? e.g. readFiles(['**/*.js', '**/*.html']). Or even shorter: readFiles(someDir, {ignore: […]}) to read all files? That’d fix the allowlist problem and still save having to name dozens of files which was my main aim.

Copy link
Owner

Choose a reason for hiding this comment

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

I'm not ready to give up on the idea of solving this with a getter function idea that easily :) Mainly because the objective overhead cost of having to maintain that duplicate file allowlist always matching the files referenced in the individual tests doesn't beat out the subjective "easier variable naming" problem.

I think your issue is addressable here. For example, I'd be happy with either of these:

// 1.
const fileToTest = await getFile('/index.js');
expect(fileToTest).toInclude(`import __SNOWPACK_ENV__ from './__snowpack__/env.js';`);
expect(fileToTest).toInclude(`import.meta.env = __SNOWPACK_ENV__;`);

// 2. (with the result memoized so that only one lookup is actually used)
expect(await getFile('/index.js')).toInclude(`import __SNOWPACK_ENV__ from './__snowpack__/env.js';`);
expect(await getFile('/index.js')).toInclude(`import.meta.env = __SNOWPACK_ENV__;`);

BUT... if you're still on board with moving towards running these tests through the JS Build API as a part of our v3 release, then I'm fine with the existing files: [] pattern for now, since the helper that I'm proposing would most likely go away when that API arrives.

Copy link
Collaborator Author

@drwpow drwpow Dec 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

BUT... if you're still on board with moving towards running these tests through the JS Build API as a part of our v3 release, then I'm fine with the existing files: [] pattern for now, since the helper that I'm proposing would most likely go away when that API arrives.

Ah yeah I’d be down for that! I think that’d be a great replacement for the current files: [] pattern.

I think your issue is addressable here. For example, I'd be happy with either of these:

Maybe I didn’t explain clearly… the current situation is more like imagine if #1 had 20+ files to keep track of, and needed to be referenced between tests.

#2 could work, but in order to be memoized we’d really need to beef it up and I’d rather just discard it in favor of the Node API later on.

See https://github.com/snowpackjs/snowpack/pull/1861/files#diff-5097166b779657484196ba597acf4521937f197da2c8803d52480c87d1fb8c75R13 for a reference on the changes… for now I changed it so that it just reads a directory.

$ = cheerio.load(files['/index.html']);
});

it('baseUrl works for <link>', () => {
expect($('link[rel="icon"]').attr('href').startsWith('/static/')).toBe(true);
expect($('link[rel="stylesheet"]').attr('href').startsWith('/static/')).toBe(true);
Expand All @@ -21,15 +26,19 @@ describe('buildOptions.baseUrl', () => {

it('import.meta.env works', () => {
// env is present in index.js
expect(indexJS).toEqual(
expect(files['/index.js']).toEqual(
Copy link
Collaborator Author

@drwpow drwpow Dec 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

This is a totally-optional test helper for reading files. You pass in any array of files to readFiles([...files], { cwd }), and you get back a map of { [fileName]: "contents" }. I found it helpful in reading a bunch of files at once, but without the annoyance of snapshots.

You’ll notice may tests just skip this helper, especially if the test is simple to begin with. Again, it‘s optional to use but I find it to be easier to read.

expect.stringContaining(`import __SNOWPACK_ENV__ from './__snowpack__/env.js';`),
);
expect(indexJS).toEqual(expect.stringContaining(`import.meta.env = __SNOWPACK_ENV__;`));
expect(files['/index.js']).toEqual(
expect.stringContaining(`import.meta.env = __SNOWPACK_ENV__;`),
);

// env is present in _dist_/index.js too
expect(distJS).toEqual(
expect(files['/_dist_/index.js']).toEqual(
expect.stringContaining(`import __SNOWPACK_ENV__ from '../__snowpack__/env.js';`),
);
expect(distJS).toEqual(expect.stringContaining(`import.meta.env = __SNOWPACK_ENV__;`));
expect(files['/_dist_/index.js']).toEqual(
expect.stringContaining(`import.meta.env = __SNOWPACK_ENV__;`),
);
});
});
5 changes: 5 additions & 0 deletions test/build/bugfix-named-import/bugfix-named-import.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,12 @@
const fs = require('fs');
const path = require('path');
const {setupBuildTest} = require('../../test-utils');

describe('bugfix: named import', () => {
beforeAll(() => {
setupBuildTest(__dirname);
});

// if this file built successfully, then the ipmort worked
it('built', () => {
const webModule = path.join(__dirname, 'build', 'web_modules', 'array-flatten.js');
Expand Down
113 changes: 0 additions & 113 deletions test/build/build.test.js
View file
Open in desktop

This file was deleted.

16 changes: 10 additions & 6 deletions test/build/cdn/cdn.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,24 +1,28 @@
const fs = require('fs');
const path = require('path');
const cheerio = require('cheerio');
const {setupBuildTest, readFiles} = require('../../test-utils');

const cwd = path.join(__dirname, 'build');
let files = {};

const html = fs.readFileSync(path.join(cwd, 'index.html'), 'utf-8');
const distJS = fs.readFileSync(path.join(cwd, '_dist_', 'index.js'), 'utf-8');
describe('CDN URLs', () => {
beforeAll(() => {
setupBuildTest(__dirname);

const $ = cheerio.load(html);
files = readFiles(['index.html', '_dist_/index.js'], {cwd});
});

describe('CDN URLs', () => {
it('HTML: preserves remote URLs', () => {
const $ = cheerio.load(files['/index.html']);
expect($('script[src^="https://unpkg.com"]')).toBeTruthy();
});

it('JS: preserves CDN URLs', () => {
expect(distJS).toEqual(
expect(files['/_dist_/index.js']).toEqual(
expect.stringContaining('import React from "https://cdn.pika.dev/react@^16.13.1";'),
);
expect(distJS).toEqual(
expect(files['/_dist_/index.js']).toEqual(
expect.stringContaining('import ReactDOM from "https://cdn.pika.dev/react-dom@^16.13.1";'),
);
});
Expand Down
Loading