filerjs / filer

Node-like file system for browsers
BSD 2-Clause "Simplified" License
613 stars 153 forks source link

npm test only running shim tests #766

Closed humphd closed 3 years ago

humphd commented 3 years ago

After merging https://github.com/filerjs/filer/commit/aacc8061cf761e7baff6fd1a842b0e9b7f0a1092 it looks like we're only running the shim tests now vs. the whole set of tests (cc @bcheidemann):

$ npm test
✨  Built in 1.05s.

tests/dist/index.js    456.49 KB    130ms

START:
07 03 2021 11:02:24.619:INFO [karma-server]: Karma v6.1.1 server started at http://localhost:9876/
07 03 2021 11:02:24.621:INFO [launcher]: Launching browsers ChromeHeadless, FirefoxHeadless with concurrency unlimited
07 03 2021 11:02:24.628:INFO [launcher]: Starting browser ChromeHeadless
07 03 2021 11:02:24.635:INFO [launcher]: Starting browser FirefoxHeadless
07 03 2021 11:02:25.046:INFO [Chrome Headless 88.0.4324.192 (Mac OS 10.14.6)]: Connected on socket OKaqpYQLHbG9xLgsAAAB with id 36504655
07 03 2021 11:02:26.586:INFO [Firefox 86.0 (Mac OS 10.14)]: Connected on socket XSU2QhUvWUmmsrQvAAAD with id 5925302
  fs shim
    ✔ should be defined
    ✔ should be an object
    ✔ should return a function when accessing fs.writeFile
    ✔ should call callback when calling fs.writeFile
    ✔ should return an object when accessing fs.promises
    ✔ should return a function when accessing fs.promises.writeFile
    ✔ should return a promise which resolves when calling fs.promises.writeFile
  path shim
    ✔ should be defined
    ✔ should be re-exposing path
    ✔ default export should be defined
    ✔ named export should be defined
    ✔ default export should be re-exposing Buffer
    ✔ named export should be re-exposing Buffer

Finished in 0.216 secs / 0.231 secs @ 11:02:26 GMT-0500 (Eastern Standard Time)

SUMMARY:
✔ 26 tests completed
SUMMARY
 0: Chrome Headless 88.0.4324.192 (Mac OS 10.14.6): Executed 13 of 504 SUCCESS (0.197 secs / 0.188 secs)
 1: Firefox 86.0 (Mac OS 10.14): Executed 13 of 504 SUCCESS (0.019 secs / 0.043 secs)
  13 test cases successful in all browsers

  Migration tests from Filer 0.43 to current
    ✓ should have a root directory
    ✓ should have expected entries in root dir
    ✓ should have correct contents for /file.txt (read as String)
    ✓ should have expected entries in /dir
    ✓ should have correct contents for /dir/file2.txt (read as Buffer)

  5 passing (12ms)
humphd commented 3 years ago

Moving the shim tests to the end of tests/index.js doesn't affect it (e.g., skips all the other tests, only runs shim tests), so pulling these shims into the main tests at all somehow disrupts how the usual filer require works.

bcheidemann commented 3 years ago

Hey, thanks for bringing this to my attention @humphd. I think it was running all of them for me but I was using npm run karma-mocha. I will look into this as soon as I have time but may have to wait until the weekend if that's okay.

humphd commented 3 years ago

Thanks. It's not a big rush, but it's something I want fixed before I ship an update. The best fix might be to do multiple builds and not bother trying to make it all work in one? Not sure.

bcheidemann commented 3 years ago

Thanks. It's not a big rush, but it's something I want fixed before I ship an update. The best fix might be to do multiple builds and not bother trying to make it all work in one? Not sure.

Do you mean to completely separate the shim tests from the rest?

humphd commented 3 years ago

Sure, we could even introduce a test that uses webpack vs. parcel to do the build, and does your tests as a separate process we run with npm-run-all, so npm test would trigger a few independent build/test runs. I think it might be hard to make these all work together, but maybe I'm wrong? Happy either way.

bcheidemann commented 3 years ago

@humphd .......... my bad! I had describe.only in my tests so as not to run all tests when I was writing them. I removed it from one but forgot to remove it from the other three spec files I wrote. For future reference, is there a way you would normally go about running only a specific describe? (I usually use jest so am not very familiar with karma-mocha)

humphd commented 3 years ago

Oh, nice. I completely missed this too. I'd have to look what the right way with Karma is, too. I also usually use Jest, so I can't remember. We used karma-mocha for Filer due to wanting to have real indexeddb in tests vs. JSDOM.

Thanks for the follow-up, I'll review your PR.

bcheidemann commented 3 years ago

I do prefer jest usually but it makes sense to use Karma for the reasons you mentioned.

By the way, I'm still happy to take a look at separating out the tests for the shims if you think this is necessary. However, I'd need to clarify what we want to get out of doing that as I'm not 100% sure why we would go down that route =P

humphd commented 3 years ago

Agreed, I don't think it's necessary, given the actual issue. I was anticipating a harder solution than removing .only :)

humphd commented 3 years ago

Fixed by #767