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

test(Karma): add e2e test for Karma files collected through sources_aspect #651

Merged
merged 1 commit into from Apr 5, 2019

Conversation

kyliau
Copy link
Collaborator

@kyliau kyliau commented Apr 4, 2019

Karma should only load explicit node_sources and dev_scripts from
its deps and runtime_deps. There should not be a case where Karma
needs to load all files in a target.

For third-party deps, an explicit file from the package should be
provided to Karma instead of the entire package.
For example, @npm//node_modules/rxjs:bundles/rxjs.umd.js should
be used instead of @npm//rxjs.

(This effectively means devserver and Karma do not know how to
serve node modules that are not in APF format, so users have to
explicitly specify the files instead of Karma blindly loading everything).

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been added / updated (for bug fixes / features)

PR Type

What kind of change does this PR introduce?

  • Bugfix
  • Feature (please, look at the "Scope of the project" section in the README.md file)
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Documentation content changes
  • Other... Please describe:

What is the current behavior?

Issue Number: N/A

What is the new behavior?

Does this PR introduce a breaking change?

  • Yes
  • No

Other information

@kyliau kyliau force-pushed the karma branch 2 times, most recently from 14fb6a2 to b40ce1a Compare April 4, 2019 19:16
Copy link
Collaborator

@gregmagolan gregmagolan left a comment

Choose a reason for hiding this comment

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

Looks right to me. The karma rule is getting confusing with all the different types of deps. I started a cleanup at one point to make the code clearer and got halfway through. Maybe time to dig that up again.

@kyliau kyliau force-pushed the karma branch 2 times, most recently from 2dd6067 to 790a4a9 Compare April 4, 2019 22:25
Copy link
Collaborator

@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.

can you consolidate this into an existing e2e and keep the same assertions?
we shouldn't continue to have new e2e for each

packages/karma/karma.conf.js Show resolved Hide resolved
@kyliau
Copy link
Collaborator Author

kyliau commented Apr 4, 2019

can you consolidate this into an existing e2e and keep the same assertions?
we shouldn't continue to have new e2e for each

done, merged into existing karma_typescript e2e.

@kyliau kyliau changed the title fix(karma): Karma should only load node_sources and dev_scripts test(Karma): add e2e test for Karma files collected through sources_aspect Apr 5, 2019
@alexeagle alexeagle merged commit 02ce264 into bazelbuild:master Apr 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants