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

installer: test installed module run correctly in the system #509

Closed
wants to merge 20 commits into from
Closed

installer: test installed module run correctly in the system #509

wants to merge 20 commits into from

Conversation

axetroy
Copy link
Contributor

@axetroy axetroy commented Jun 19, 2019 •
edited
Loading

Make sure the installed modules are working correctly on the system.

closes #500
wait for #510 be merged, and this PR should ready for review.

@axetroy axetroy changed the title installer: test installed module run correctly installer: test installed module run correctly in system Jun 19, 2019
@axetroy axetroy marked this pull request as ready for review June 19, 2019 09:23
@bartlomieju
Copy link
Member

@axetroy can you update the rest of installer test cases to install either args.ts or echo.ts? It makes no sense to install file_server during test pipeline - it downloads a lot of dependent files.

@axetroy axetroy changed the title installer: test installed module run correctly in system installer: test installed module run correctly in the system Jun 19, 2019
@axetroy
Copy link
Contributor Author

axetroy commented Jun 19, 2019

@axetroy can you update the rest of installer test cases to install either args.ts or echo.ts? It makes no sense to install file_server during test pipeline - it downloads a lot of dependent files.

@bartlomieju
args.ts and echo.ts used to test if installer passes parameters correctly to the installed module

Are you saying that the file_server module is no longer installed as a test?

@bartlomieju
Copy link
Member

Are you saying that the file_server module is no longer installed as a test?

It is, but it's a bad module to install in pipeline. There is no need to install a module which has a lot of deps.

bartlomieju
bartlomieju approved these changes Jun 19, 2019
View reviewed changes
Copy link
Member

@bartlomieju bartlomieju left a comment

Choose a reason for hiding this comment

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

LGTM

bartlomieju
bartlomieju reviewed Jun 19, 2019
View reviewed changes
installer/mod.ts
@@ -241,6 +241,11 @@ export async function install(
}
}

// if install local module
if (!/^https?:\/\//.test(moduleUrl)) {
Copy link
Member

Choose a reason for hiding this comment

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

Yikes, this should have been merged in #499

Copy link
Contributor Author

@axetroy axetroy Jun 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I miss this. :)

Copy link
Contributor Author

@axetroy axetroy Jun 19, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I will add the relevant tests to install the local module.

@bartlomieju bartlomieju mentioned this pull request Jun 19, 2019
Merged
@axetroy
Copy link
Contributor Author

axetroy commented Jun 20, 2019

move to #512

@axetroy axetroy closed this Jun 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bartlomieju bartlomieju bartlomieju approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Installer: missing real test in different platforms
2 participants
@axetroy @bartlomieju