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.

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

Refactor system tests #4755

Merged
merged 3 commits into from May 9, 2020
Merged

Refactor system tests #4755

merged 3 commits into from May 9, 2020

Conversation

jeddy3
Copy link
Member

@jeddy3 jeddy3 commented May 8, 2020

Which issue, if any, is this issue related to?

This lays the foundation for being able to test bundled code against real-world examples, closing #2454 and #3935.

Is there anything in the PR that needs further explanation?

The system tests were designed to:

... assert that we end up with some expected output, given a configuration and a stylesheet. They should reproduce real use-cases and verify that those use-cases work as expected.

Somewhere along the line, we muddied the waters. Tests that were not related to real-world cases, didn't use real-world configs, nor snapshotted the results were added. The tests often duplicated specific fixtures from the __tests__/fixtures directory into the config.json and stylesheet.css files of the system tests.

This has led to a fragmentation of the tests.

Additionally, the landscape has changed. The original system tests were added before:

  • the autofix feature existed
  • the Bootstrap and Sanitize.css teams used stylelint
  • our refactoring to produce a browser bundle

I've used this opportunity to update the system tests to reflect this.

In this pull request, I've:

  • moved the non-system tests into lib/__tests__, making use of existing fixtures or creating new ones where appropriate
  • moved replaceBackSlashes into testUtils, cleaning up the __tests__ directory
  • removed the duplicated replaceBackSlashes function from systemTestUtils.js
  • updated the system tests to:
    • test three real-world examples:
      • a Bootstrap SCSS source file and their config
      • the Sanitize.css source file and their config
      • the Zen Garden CSS and the standard config (as an example of using running stylelint to fix an old code base)
    • support snapshotting fixed code
    • support using the code and config properties, rather than file and configFile ones, to avoid filesystem access

@jeddy3 jeddy3 force-pushed the refactor-system-tests branch 2 times, most recently from 3599722 to 90056e9 Compare May 8, 2020 15:31
const standalone = require('../standalone');

const copyFile = promisify(fs.copyFile);
const readFile = promisify(fs.readFile);
Copy link
Member

Choose a reason for hiding this comment

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

I believe it's possible to do in Node.js 10+:

const { copyFile } = require('fs').promises;

or

const { copyFile } = require('fs/promises');

https://nodejs.org/api/fs.html#fs_fs_promises_api
https://nodejs.org/api/fs.html#fs_fspromises_copyfile_src_dest_mode

Copy link
Member Author

Choose a reason for hiding this comment

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

That's odd. I originally wrote it using the promise API but refactored it after the node/no-unsupported-features ESLint plugin warned against using it. I'll take another look because it appears to be a stable API in node@10.

Copy link
Member

Choose a reason for hiding this comment

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

Linter plugin false positive mysticatea/eslint-plugin-node#174

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll use the API and disable the ESLint warning for now.

const replaceBackslashes = require('../lib/testUtils/replaceBackslashes');

const copyFile = promisify(fs.copyFile);
const readFile = promisify(fs.readFile);
Copy link
Member

Choose a reason for hiding this comment

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

Same as comment above. promisify is not needed here.

Comment on lines 55 to 61
results: results.map(({ _postcssResult, ...rest }) => {
delete rest.source;

return rest;
}),
output,
...rest,
Copy link
Member

Choose a reason for hiding this comment

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

For safety let's use different names for rest, so we're not confused in results scope what rest we're dealing with.

Also we can use single way of removing unneeded property:

		results: results.map(({ _postcssResult, source, ...rest }) => {
			return rest;
		}),

Or:

		results: results.map((result) => {
			delete result.source;
			delete result._postcssResult;

			return result;
		}),

@jeddy3
Copy link
Member Author

jeddy3 commented May 9, 2020

Changes made.

Copy link
Member

@hudochenkov hudochenkov left a comment

Choose a reason for hiding this comment

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

Good work!

Copy link
Member

@m-allanson m-allanson left a comment

Choose a reason for hiding this comment

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

Good stuff.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants