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

Remove depth limit from chokidar options #1697

Merged
merged 9 commits into from Mar 21, 2019

Conversation

marcofugaro
Copy link
Contributor

  • This is a bugfix
  • This is a feature
  • This is a code refactor
  • This is a test update
  • This is a docs update
  • This is a metadata update

Motivation / Use-Case

The watchContentBase option would only watch only the root folder, this PR removes that limitation.

Discussion in #1694

The watchContentBase option will now watch the folder recursively

@codecov
Copy link

codecov bot commented Mar 4, 2019

Codecov Report

Merging #1697 into master will increase coverage by 2.22%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1697      +/-   ##
==========================================
+ Coverage   83.85%   86.08%   +2.22%     
==========================================
  Files           8        8              
  Lines         539      539              
  Branches      162      162              
==========================================
+ Hits          452      464      +12     
+ Misses         70       62       -8     
+ Partials       17       13       -4
Impacted Files Coverage Δ
lib/Server.js 82.19% <ø> (+3.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 09de43e...d179f45. Read the comment docs.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Looks good, but need tests for this

@marcofugaro
Copy link
Contributor Author

@evilebottnawi
For now I've made the test that edits a nested file, but I should check that chokidar has been triggered and the browser has been refreshed. How do i go about testing this?

Could you give me more directions?

@alexander-akait
Copy link
Member

No need check is browser refreshed, anyway if you do this will be great (example e2e tests https://github.com/webpack/webpack-dev-server/blob/master/test/Client.test.js).

Main test is mocking chokidar and check event was handled (

watcher.on('change', () => {
). More infromation about mocking https://jestjs.io/docs/en/mock-functions.html

@marcofugaro
Copy link
Contributor Author

@evilebottnawi alright I wrote some code that should work, however I am having some troubles with connecting puppeteer to the server, am I doing something wrong?

image

@alexander-akait
Copy link
Member

maybe you already have app on 9001 port? Or you we need run test in serial

@marcofugaro
Copy link
Contributor Author

@evilebottnawi jest is already being run with the --runInBand, so it should run one at a time.

Also no, I don't have anything on the 9001 port, it still gives the error even if I change the port.

@hiroppy
Copy link
Member

hiroppy commented Mar 11, 2019

@marcofugaro This test code should be fixed like below.

diff --git a/test/ContentBase.test.js b/test/ContentBase.test.js
index 4813372..70b57eb 100644
--- a/test/ContentBase.test.js
+++ b/test/ContentBase.test.js
@@ -20,7 +20,6 @@ const contentBaseOther = path.join(
 describe('ContentBase', () => {
   let server;
   let req;
-  afterEach(helper.close);
 
   describe('to directory', () => {
     beforeAll((done) => {
@@ -32,15 +31,21 @@ describe('ContentBase', () => {
       addEntries(config, options);
       server = helper.start(
         config,
-        {
+        Object.assign({}, options, {
           contentBase: contentBasePublic,
           watchContentBase: true,
-        },
+        }),
         done
       );
       req = request(server.app);
     });
 
     it('Request to index', (done) => {
       req.get('/').expect(200, /Heyo/, done);
     });
@@ -54,7 +59,8 @@ describe('ContentBase', () => {
 
       runBrowser().then(({ page, browser }) => {
         // wait for first load
-        page.goto('http://localhost:9001').then(() => {
+        page.goto('http://0.0.0.0:9001').then(() => {
+          done(); // TODO: change this
           // page reloaded after the first load,
           // meaning it watched the file correctly
           page.on('load', () => {
@@ -82,6 +88,12 @@ describe('ContentBase', () => {
       req = request(server.app);
     });
 
+    afterAll((done) => {
+      helper.close(() => {
+        done();
+      });
+    });
+
     it('Request to first directory', (done) => {
       req.get('/').expect(200, /Heyo/, done);
     });
@@ -103,6 +115,12 @@ describe('ContentBase', () => {
       req = request(server.app);
     });
 
+    afterAll((done) => {
+      helper.close(() => {
+        done();
+      });
+    });
+
     it('Request to page', (done) => {
       req
         .get('/other.html')
@@ -123,6 +141,12 @@ describe('ContentBase', () => {
       req = request(server.app);
     });
 
+    afterAll((done) => {
+      helper.close(() => {
+        done();
+      });
+    });
+
     it('Request to page', (done) => {
       req
         .get('/foo.html')
@@ -147,6 +171,12 @@ describe('ContentBase', () => {
       req = request(server.app);
     });
 
+    afterAll((done) => {
+      helper.close(() => {
+        done();
+      });
+    });
+
     it('Request to page', (done) => {
       req.get('/other.html').expect(200, done);
     });
@@ -167,6 +197,12 @@ describe('ContentBase', () => {
       req = request(server.app);
     });
 
+    afterAll((done) => {
+      helper.close(() => {
+        done();
+      });
+    });
+
     it('Request to page', (done) => {
       req.get('/other.html').expect(404, done);
     });
@@ -184,6 +220,12 @@ describe('ContentBase', () => {
       req = request(server.app);
     });
 
+    afterAll((done) => {
+      helper.close(() => {
+        done();
+      });
+    });
+
     it('Request foo.wasm', (done) => {
       req.get('/foo.wasm').expect('Content-Type', 'application/wasm', done);
     });

@marcofugaro
Copy link
Contributor Author

Thank you @hiroppy that fixed my problem, however i could not get socket.on('connection', to trigger during tests, so I ended up lisstening to chokidar's change event.

Now the test works @evilebottnawi!

@hiroppy
Copy link
Member

hiroppy commented Mar 20, 2019

/cc @evilebottnawi PTAL

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

4 participants