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

fix: validate preserveSymlinks tsconfig option is not set #171

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

jbedard
Copy link
Member

@jbedard jbedard commented Oct 11, 2022

I'm finding this a bit awkward to test because typescript is only fetched from BUILD.typescript which isn't really accessible from tests. But every test out there ensures it at least works with valid config, and I've tested the case this is catching manually. @thesayyn I don't understand how you tested workers with such minimal mocking of typescript?

@jbedard jbedard requested a review from thesayyn October 11, 2022 22:57
@jbedard jbedard force-pushed the 63-tsconfig-validate-preserve branch from 2e0af20 to d7d54c1 Compare October 11, 2022 22:57
@thesayyn
Copy link
Member

function mock(name, exports) {
const p = path.resolve(name);
require.cache[p] = {
id: name,
file: p,
loaded: true,
exports: exports
};
const realres = mod._resolveFilename;
mod._resolveFilename = function (request, parent) {
if (request == name) {
return p;
}
return realres(request, parent);
};
}
is the magic function that does it all. I only needed to mock the typescript and worker package partially to test fsTree, not the worker itself. you might wanna bring in a test runner with mocking ability which i didn't want to do in the first place.

Copy link
Member

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

Yeah I had the same issue in https://github.com/bazelbuild/rules_nodejs/tree/stable/packages/typescript/test/ts_project/validation - it's a challenge to write a negative test case for validation failures.

The logic is so simple to read and understand that I think it's fine to skip automated tests for this one.

@alexeagle alexeagle merged commit 7582e06 into aspect-build:main Oct 12, 2022
@jbedard
Copy link
Member Author

jbedard commented Oct 12, 2022

@thesayyn that's what I was looking at, but the number of functions needing to be mocked was just way too high

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

Successfully merging this pull request may close these issues.

3 participants