-
Notifications
You must be signed in to change notification settings - Fork 142
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
td.replace() on an un-parseable source will claim the module is missing instead of printing the parse error #320
Comments
As I get further into this, I'm not sure how to proceed. Do you have any tips on using testdouble with webpacked source. If quibble looking for a specific filename to include and webpack has changed the name during the build, this is no bueno. |
Separately, depending on the nature of the thing being tested you might consider only running your unit tests in a Node.js runtime. I used to run mine under Node and then again through a browser environment, but have found over the years that the likelihood of the second run catching anything was nearly zero. (Whether this is tenable depends entirely on what you're publishing) I can understand why this would be confusing, though, since (it's my understanding) most folks compile the raw JS of their npm dependencies down for their webpack bundles. While this will work for testdouble.js, the warnings of passing a string to cc/ing @jasonkarns for his opinion on what we should do here |
Closing as it isn't an issue per se, but would be open to changing how the warning works so it doesn't require web users to find |
@fantapop typically, source isn't webpacked prior to running tests, so the test suite (and frameworks) would still have access to the original source tree. Assuming you're webpacking everything down to a single bundle, you wouldn't be able to unit test into that bundle and would only be able to run end-to-end tests of the webpacked bundle. (This is almost universally true regardless of which browser bundler being used, or the test framework.) The only exceptions to this are if you use webpack to bundle the test suite as well. Thus the tests would be webpacked along with the source, and could run in browser. Though again, No ideas yet on how to improve the error/warning but definitely room for improvement there. |
Thanks for your replies. Sorry for the confusion, I am using testdouble in node.js. The project is based on this library: https://github.com/serverless-heaven/serverless-webpack. I was able to work around the webpack issue by separately running babel to convert the es6 module syntax back down to require. I couldn't figure out how to do this with webpack correctly however Its most likely possible. Regarding the issue I filed, do you not consider this a bug? This code path is still executed in the node js environment. If I do something like change the name of the module I'm doubling:
It correctly falls through to using the project root to find the module. However, If the module exists, uses only require(), but has an error in it, that error is thrown here but is masked. For example if I interject ```asdfasfdasdf;`` at the top of the file, there error thrown is:
when it should be:
An easy fix for this would be something like this in
|
I'm sorry, but I'm struggling to follow you. Could you provide a minimal reproducible example or a failing test? |
Wow, thanks for the clear example. I completely understand your point now and this is definitely a bug. Looking at just the stack trace, I want to blame the resolve module, but I can't be sure just yet. |
Did a few minutes of research to find that resolve is actually loading the file (this is news to me, as I wasn't aware that Node's path resolution algorithm actually required one to literally read the file), and is blowing up at this line |
I believe this should now be fixed in td.js and will cut a release to that effect, by removing substack's resolve package and using the built-in As I dug in, I remembered quibble also depends on the Since the purpose of this |
Ok, Landed in 3.3.1 & fixed by 9f51110 Try this and let me know if it works for you |
Thanks a ton for working on this. I tested it with my repro and it does the job. I certainly don't understand the nuances of supporting both the browser and node with this code path here but I'm wondering why you didn't want to go with my suggested fix of checking for errors caused by the original include. i.e.
The new fix you put into place is actually repeating the work of the first include in order to propagate the error out to the user. You can see this by marking up the dist code with console log statements such as this:
It produces the following output:
Also, notable is that the comment: |
I've just completed an upgrade to my project to use webpack and with it module import statements. Our tests stopped working. The failure I was seeing was:
After a bit digging, I found this bit of code:
My files were failing to require but the error here is getting squashed under the assumption that the file could not be found. After adding more verbose logging I can see the following error:
Maybe you can inspect the error that is getting thrown and look for file not found or something and throw all others.
The text was updated successfully, but these errors were encountered: