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

Jest tests #310

Merged
merged 2 commits into from Sep 9, 2017

Conversation

Projects
None yet
5 participants
@richardwillars
Contributor

richardwillars commented Sep 1, 2017

Switched from tape to Jest and started writing tests for everything in the src/core directory. A lot more tests to follow, this is work in progress (I just created a PR so you can see progress) so please don't merge just yet!

@richardwillars richardwillars referenced this pull request Sep 1, 2017

Closed

Bump to node v6 #311

@arturi

This comment has been minimized.

Collaborator

arturi commented Sep 1, 2017

👍 Even though I like the simplicity of tape, I heard good things about Jest, and I was about to suggest we switch myself. So I vote yes to Jest, and thank you for the amazing work on writing more tests! 🥇 Great news.

@richardwillars

This comment has been minimized.

Contributor

richardwillars commented Sep 1, 2017

@arturi Although this work is nowhere near complete (still got loads to do in src/core/core and all the plugins), it's probably worth considering merging it at this point to stop it going out of sync with the codebase too much. I'll then break down the future tests into smaller PRs.

One thing to note: I'm unable to update the package-lock.json. New dependencies are in package.json but I can't get it to update the lock file.. someone else might have better luck!

@richardwillars richardwillars changed the title from Jest tests [WIP] to Jest tests Sep 1, 2017

@arturi

This comment has been minimized.

Collaborator

arturi commented Sep 3, 2017

@goto-bus-stop @kvz @ifedapoolarewaju are you ok with moving to Jest for tests? We already use Jest in Uppy Server, so makes sense. If there are concerns though, this is the time to voice them ;)

@ifedapoolarewaju

This comment has been minimized.

Collaborator

ifedapoolarewaju commented Sep 3, 2017

sounds good to me 👍

@kvz

This comment has been minimized.

Member

kvz commented Sep 3, 2017

@goto-bus-stop

This comment has been minimized.

Member

goto-bus-stop commented Sep 3, 2017

i'm not super familiar with jest yet, but no objections from me. agree the consistency with uppy-server is good to have!

@goto-bus-stop

This comment has been minimized.

Member

goto-bus-stop commented Sep 8, 2017

@richardwillars Just a heads up, there's a typo in your local git email (a triple l) that's preventing github from attributing your commits to your github account

Agree about it being a good idea to merge quickly, then afterward we need to port the removed tests for:

  • Transloadit
  • FileInput (very minor)
  • GoogleDrive provider
  • did i miss any?

@arturi do you think this is ok to merge today?

@arturi

This comment has been minimized.

Collaborator

arturi commented Sep 8, 2017

do you think this is ok to merge today?

@goto-bus-stop I think so, bumping the node version and looking out 👀 for potential fires first, then let’s do it?

@richardwillars

This comment has been minimized.

Contributor

richardwillars commented Sep 8, 2017

@goto-bus-stop Nooooooooo! Good spot though.. how the hell did I spend my name wrong. That's 2 years of open source contributions down the drain :|

@goto-bus-stop

This comment has been minimized.

Member

goto-bus-stop commented Sep 8, 2017

Add jest tests
Adds tests for src/core/Utils

Ports translator tests to jest

Adds tests for core/index

Added code coverage report

Adds tests for core/UppySocket

Adds tests to src/core and cleanup of test directory

Switched from import to require as the project supports node v4

Runs tests using Babel for backwards compatibilty

Adds src/core state tests

Adds src/core reset & close tests

Updates stagnant snapshot

Adds tests for preprocessors and postprocessors in src/core/Core.js

Adds tests for uploaders and metadata in src/core/Core

Adds tests for adding and removing files in src/core/Core

Adds test for getFile
@richardwillars

This comment has been minimized.

Contributor

richardwillars commented Sep 8, 2017

Squashed them all into one before I read your comment! Seems to have worked anyway 😌

Still got a few more tests to write in core.js, then I'll move onto the plugins. Won't take long to reintroduce the old tests. I'll wait until after the merge before continuing..

@@ -0,0 +1,38 @@
import Core from '../../src/core/index.js'
import russian from '../../src/locales/ru_RU.js'

This comment has been minimized.

@arturi

arturi Sep 8, 2017

Collaborator

should we be using import here if everywhere else we are using require? IT probably works because Babel, and maybe its fine, but thought I’d bring it up.

import Core from '../../src/core/index.js'
import russian from '../../src/locales/ru_RU.js'
import english from '../../src/locales/en_US.js'

This comment has been minimized.

@arturi

arturi Sep 8, 2017

Collaborator

Locale files are probably outdates, because locale settings are per-plugin now. Not a major issue, but just to keep this in mind :)

@arturi arturi merged commit 5d30474 into transloadit:master Sep 9, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arturi

This comment has been minimized.

Collaborator

arturi commented Sep 9, 2017

Merged! Thanks again for an amazing work, @richardwillars! All was good on my machine, waiting for Travis now.

@kvz

This comment has been minimized.

Member

kvz commented Sep 11, 2017

Amazing 😍

@richardwillars richardwillars deleted the richardwillars:jest-tests branch Sep 11, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment