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

npm test only running shim tests #766

Closed
humphd opened this issue Mar 7, 2021 · 10 comments
Closed

npm test only running shim tests #766

humphd opened this issue Mar 7, 2021 · 10 comments

Comments

@humphd
Copy link
Contributor

humphd commented Mar 7, 2021

After merging aacc806 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
Copy link
Contributor Author

humphd commented Mar 7, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

humphd commented Mar 15, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

humphd commented Mar 17, 2021

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
Copy link
Contributor

@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
Copy link
Contributor Author

humphd commented Mar 20, 2021

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
Copy link
Contributor

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
Copy link
Contributor Author

humphd commented Mar 20, 2021

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

humphd pushed a commit that referenced this issue Mar 21, 2021
* test: replace describe.only in shim tests with describes so all tests run

* test: move require chai to top of file for consistency with other tests

* chore: npm install

* test: replace describe.only in shim tests with describes so all tests run

* test: move require chai to top of file for consistency with other tests

* chore: npm install

* Revert "chore: npm install"

This reverts commit cddeef4.

* Revert "test: move require chai to top of file for consistency with other tests"

This reverts commit 40df791.
@humphd
Copy link
Contributor Author

humphd commented Mar 21, 2021

Fixed by #767

@humphd humphd closed this as completed Mar 21, 2021
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

No branches or pull requests

2 participants