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

Chore: Test fixes for CascadingConfigArrayFactory #17

Merged
merged 18 commits into from
Nov 17, 2020
Merged

Conversation

nzakas
Copy link
Member

@nzakas nzakas commented Oct 28, 2020

Last bunch of tests! 🎉

Note that I had to port over a bunch of fixtures from the main eslint repo so that makes this PR look more involved than it actually is.

Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

Did you intend to add tests/lib/cascading-config-array-factory.js to package.json's test command as part of this PR? I pulled this branch locally, added that, and got 11 test failures. Checking with you before I dig in case those just aren't finished yet.

beforeEach(async () => {

({ prepare, cleanup, getPath } = createCustomTeardown({
cwd: homeDir,
Copy link
Member

Choose a reason for hiding this comment

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

Since this uses the actual home directory now, some of these tests could fail if someone already has a ~/.eslintrc.json file there. Thankfully the teardown library appears to leave the file alone if it already exists, so at least it's not destructive:

$ echo '!@#$%^&*()' > ~/.eslintrc.json
$ npm test
# failures
$ cat ~/.eslintrc.json
!@#$%^&*()

Unless you have a bright idea for a way to test this without touching the home directory, I wouldn't be totally opposed to skipping the home directory tests outside CI. It's a little weird, but ideally people won't be touching the code in this repository anymore once you're done with the migration.

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'd say at this point let's just let the weird tests be weird. I can't wrap my brain around most of what the tests are doing at this point, so I'd call it a success when stuff starts passing even if it's not ideal.

Copy link
Member

Choose a reason for hiding this comment

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

Looks like this uses a fake home dir, created in the project's parent dir:

beforeEach(() => {
uniqueHomeDirName = `home_${++uid}`;
homeDir = path.join(__dirname, `../../../${uniqueHomeDirName}`);
warnings = [];
sinon.stub(os, "homedir").returns(homeDir);
process.on("warning", onWarning);
});

Should we maybe avoid creating and deleting (potentially already existing and unrelated) directories there, by setting homeDir to a path inside systemTempDir?

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 really don't want to be changing how the existing tests work. I've had a hard enough time converting these tests, so I'd rather just keep everything as close to original as possible.

@nzakas
Copy link
Member Author

nzakas commented Nov 2, 2020

Huh. I thought I had updated the test command. I wonder if I just forgot to push my last commit. I’ll take a look

@nzakas
Copy link
Member Author

nzakas commented Nov 2, 2020

Apparently not, I'm seeing the test failures now, too. Unfortunately, a bunch of the failures look like they're the product of tests with implicit dependencies on the results of other tests. 😢 I'll have to take a look tomorrow.

@nzakas
Copy link
Member Author

nzakas commented Nov 3, 2020

There are two tests failing on Windows in the CI but not locally when I test (either in Git Bash or the Windows command line). At this point, I think the best path forward is just to turn off those two tests.

@nzakas nzakas marked this pull request as draft November 6, 2020 19:35
@nzakas nzakas marked this pull request as ready for review November 6, 2020 19:58
@mdjermanovic
Copy link
Member

Could be because this directory is missing:

https://github.com/eslint/eslint/tree/master/tests/fixtures/config-hierarchy/plugins

You probably have it locally, but is git-ignored here:

node_modules/

eslint/eslint's .gitignore has /node_modules, so that directory isn't ignored.

It seems that ubuntu tests are passing because mocha tests/lib/**/*.js doesn't run all tests there:

281 passing

I see 573 passing locally on Windows. We should probably add quotes:

"test": "mocha -R progress -c \"tests/lib/**/*.js\"",

@nzakas
Copy link
Member Author

nzakas commented Nov 7, 2020

Wow, nice work catching that. I'll dig in some more.

You can probably tell my brain is completely fried from migrating all of these tests. Another set of eyes was definitely needed.

@nzakas
Copy link
Member Author

nzakas commented Nov 7, 2020

All green. You are my hero, @mdjermanovic. 🎉

nzakas and others added 2 commits November 12, 2020 18:52
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Copy link
Member

@btmills btmills left a comment

Choose a reason for hiding this comment

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

I ran tests locally and couldn't find anything else it wasn't cleaning up. LGTM 👍

.gitignore Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
Copy link
Member

@mdjermanovic mdjermanovic left a comment

Choose a reason for hiding this comment

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

LGTM!

Co-authored-by: Brandon Mills <btmills@users.noreply.github.com>
@nzakas nzakas merged commit f30bb49 into main Nov 17, 2020
@nzakas nzakas deleted the ccaf-test-fixes branch November 17, 2020 01:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants