-
Notifications
You must be signed in to change notification settings - Fork 251
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: log MODULE_NOT_FOUND when the karma.conf.js does not exist #1145
Conversation
Thanks for making a pull request for this! |
try { | ||
const userConfig = requireModule(configFileName); | ||
log.debug('Importing config from "%s"', configFileName); |
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.
Is there a reason why you moved this logging statement?
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 thought that you don't like to have debug
if there is an error, since you told:
When I set an incorrect path to my karma.conf.js file or if I don't copy the karma.conf.js file to my sandbox. I see the following logging:
09:34:03 (84321) INFO stryker-karma.conf.js Importing config from "C:\dev\myapp\.stryker-tmp\sandbox5403836\karma.conf.js"
09:34:03 (84321) ERROR stryker-karma.conf.js Could not read karma configuration from karma.conf.js. { code: 'MODULE_NOT_FOUND' }
i understand you want 'Importing config from "%s"', configFileName
even if error happens?
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.
Yes, I think it would be smart to still log that we're going to try to read the config file
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.
Sure. Changing it back right now
I wanted to test this code too, but I didn't manage to do it :( |
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.
Thanks for fixing. I just have a small suggestion to improve the message.
It might be that the karma.config.js file location is correct, but not added to the sandbox.
@@ -21,7 +21,11 @@ function setUserKarmaConfigFile(config: Config, log: Logger) { | |||
userConfig(config); | |||
config.configFile = configFileName; // override config to ensure karma is as user-like as possible | |||
} catch (error) { | |||
log.error(`Could not read karma configuration from ${globalSettings.karmaConfigFile}.`, error); | |||
if (error.code === 'MODULE_NOT_FOUND') { | |||
log.error(`Unable to find karma config at "${globalSettings.karmaConfigFile}". Please check your stryker config.`); |
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.
Could we add configFileName
to the mix?
Unable to find karma config at "${globalSettings.karmaConfigFile}" (tried to load from ${configFileName}}. Please check your stryker config. You might need to make sure the file is included in the sandbox directory.
@kmdrGroch
Shall I help with that? That way you can see how to do it in the future 👍 |
@nicojs sure. I know a bit how to write tests (even with mocha), but i couldn't write working one ._. Or maybe I tested bad thing |
I've added the test. Can you make sense of it? Also fixed a new compile error. |
I would never think it should have been make like that O.o const expectedKarmaConfigFile = 'foobar.conf.js';
sut.setGlobals({ karmaConfigFile: expectedKarmaConfigFile });
sut(config);
expect(logMock.error).calledWith(`Unable to find karma config at "foobar.conf.js" (tried to load from ${path.resolve(expectedKarmaConfigFile)})`); and it didnt work. I need to read more about mocks i suppose. Or practice makes master. :) But since it works, i think ready to merge, and thanks a lot <3 |
Can fix #1098