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

refactor(server): refactor bundleResource in lib/server.js #3029

Merged
merged 1 commit into from Jun 8, 2018
Merged

refactor(server): refactor bundleResource in lib/server.js #3029

merged 1 commit into from Jun 8, 2018

Conversation

lusarz
Copy link
Contributor

@lusarz lusarz commented Jun 5, 2018

Pull request refactor code around static/context.js and static/karma.js bundling. Main changes:

  • move code responsible for bundle resources with browserify into lib/utils/bundle-utils
  • move responsibility to check if bundled files exists into BundleUtils.bundleResourceIfNotExist method
  • overall refactoring
  • adding unit tests for created methods

lib/server.js Outdated
function bundleResource (inPath, outPath) {
if (fs.existsSync(outPath)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved responsibility to check if outPath file already exists to bundleResource method.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please put this comment in the git commit description.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You mean pull request description ?

if (fs.existsSync(karmaJsPath) && fs.existsSync(contextJsPath)) {
startWebServer()
} else {
this.log.info('Front-end scripts not present. Compiling...')
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole else block was actually not covered by unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

So this test does not work?

it('should compile static resources on first run', function (done) {

Copy link
Contributor Author

@lusarz lusarz Jun 6, 2018

Choose a reason for hiding this comment

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

Sorry, I was in a hurry. True, this test covers bundleResource invoke in else block, but it was possible to remove this line:

.then(startWebServer)

and no unit test fails

Copy link
Contributor

Choose a reason for hiding this comment

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

Now that I look more carefully the else case test is not correct because fs is not injected and the test runs in the same filesystem location every time. If the file system has the static files from a previous run or from a use of karma the test will pass even if bundling code does not work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated it a bit, now everything should be tested.

lib/server.js Outdated
function bundleResource (inPath, outPath) {
if (fs.existsSync(outPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please put this comment in the git commit description.

lib/server.js Outdated
process.exitCode = 1
process.kill(process.pid, 'SIGINT')
return Promise.all([
bundleResource(path.join(__dirname, '/../client/main.js'), path.join(__dirname, '/../static/karma.js')),
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the code would be clearer if you create function to compute the paths:

  bundleResource(['/../client/main.js', '/../static/karma.js'].map(absPath));

If you also pass the fs:

  bundleResource(fs, ['/../client/main.js', '/../static/karma.js'].map(absPath));

then we can unit test the bundleResource() properly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe a bit other way, but done :)

@lusarz lusarz requested a review from johnjbarton June 8, 2018 06:16
Copy link
Contributor

@johnjbarton johnjbarton left a comment

Choose a reason for hiding this comment

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

Thanks!

@johnjbarton johnjbarton merged commit 011a90c into karma-runner:master Jun 8, 2018
@lusarz lusarz deleted the refactor-server-bundle-resource branch June 9, 2018 05:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants