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

feat(InputfileResolver): exclude online files from globbing #194

Merged
merged 6 commits into from
Dec 30, 2016

Conversation

Kattoor
Copy link
Contributor

@Kattoor Kattoor commented Dec 27, 2016

This pull request fixed issue #90

@Kattoor Kattoor requested review from nicojs and simondel December 27, 2016 14:59
Copy link
Member

@simondel simondel left a comment

Choose a reason for hiding this comment

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

Looks good! You may want to change the functionisWebUrl

@@ -7,13 +7,18 @@ const log = log4js.getLogger('InputFileResolver');

const DEFAULT_INPUT_FILE_PROPERTIES = { mutated: false, included: true };

function isWebUrl(pattern: string): boolean {
return pattern.indexOf('http://') === 0 || pattern.indexOf('https://') === 0;
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 also use pattern.startsWith('http://') 👍

Copy link
Member

Choose a reason for hiding this comment

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

Turns out this is a part of ES6, thanks for the heads-up @Kattoor

if (mutationArray) {
mutationArray.forEach(mutation => {
if (isWebUrl(mutation)) {
throw new Error(`Cannot mutate web url "${mutation}".`)
Copy link
Member

Choose a reason for hiding this comment

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

@simondel simondel self-assigned this Dec 28, 2016
@simondel
Copy link
Member

This may not work when we copy the files to the sandbox because the file is not on the file system. It may be smart to move isWebUrl to a util class so we can use it multiple times.

Copy link
Member

@nicojs nicojs left a comment

Choose a reason for hiding this comment

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

@Kattoor you forgot to make changes to the Sandbox. See fillSandbox method.

The Sandbox class represents an isolated test runner with its own copy of all the files, It should not contain copies of web url's, those can safely be shared across active test runners.

@simondel
Copy link
Member

I'll fix this

Copy link
Member

@nicojs nicojs left a comment

Choose a reason for hiding this comment

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

Looks good, just one small thing.

if (isOnlineFile(file.path)) {
this.fileMap[file.path] = file.path;
return Promise.resolve();
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you add an else here? That is a bit cleaner and safer IMHO.

@nicojs nicojs merged commit a114594 into master Dec 30, 2016
@nicojs nicojs deleted the exclude-online-files-globbing branch December 30, 2016 06:45
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