-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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: patch karma to allow loading virtual packages #3531
fix: patch karma to allow loading virtual packages #3531
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
@googlebot I signed it! |
CLAs look good, thanks! ℹ️ Googlers: Go here for more info. |
✅ Build karma 309 completed (commit f59ad662fb by @snasirca) |
✅ Build karma 2707 completed (commit f59ad662fb by @snasirca) |
✅ Build karma 308 completed (commit f59ad662fb by @snasirca) |
21b8233
to
76a410b
Compare
✅ Build karma 2708 completed (commit 693d0326da by @snasirca) |
✅ Build karma 310 completed (commit 693d0326da by @snasirca) |
✅ Build karma 309 completed (commit 693d0326da by @snasirca) |
What would be the best way to add regression coverage to this change? |
Thanks for fixing this! A unit test with the problematic path similar to this should be enough. |
I just ran into this issue yesterday and would like to get this merged as soon a possible. I can write the unit test if needed. |
76a410b
to
dc4fc1f
Compare
dc4fc1f
to
90cce10
Compare
✅ Build karma 313 completed (commit fa250594dc by @snasirca) |
✅ Build karma 2712 completed (commit fa250594dc by @snasirca) |
✅ Build karma 314 completed (commit fa250594dc by @snasirca) |
✅ Build karma 2713 completed (commit d95e16971b by @snasirca) |
✅ Build karma 315 completed (commit d95e16971b by @snasirca) |
✅ Build karma 314 completed (commit d95e16971b by @snasirca) |
@devoto13 can you review this? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One comment for consistency, otherwise LGTM.
@@ -220,10 +220,10 @@ function createKarmaMiddleware ( | |||
}) : [] | |||
|
|||
return data | |||
.replace('%SCRIPTS%', scriptTags.join('\n')) | |||
.replace('%SCRIPTS%', () => scriptTags.join('\n')) | |||
.replace('%CLIENT_CONFIG%', 'window.__karma__.config = ' + JSON.stringify(client) + ';\n') | |||
.replace('%SCRIPT_URL_ARRAY%', 'window.__karma__.scriptUrls = ' + JSON.stringify(scriptUrls) + ';\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe this should be changed to function as well to cover this branch. Please change it and add a corresponding test.
@snasirca wondering about the status of this. I'm running into the same issue and found this potential fix. |
Sorry about the delay. This hasn't dropped off of radar. I'm on vacation and back by September 20. That'll be the earliest I can address the code review feedback. |
@snasirca Thank you for the hard work. I can confirm this is working for our project. |
This fix was subsequently merged in #3663 |
Fixes #3530