close
The Wayback Machine - https://web.archive.org/web/20220512145501/https://github.com/lukasoppermann/html5sortable/issues/259
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 & review test #259

Open
1 of 2 tasks
lukasoppermann opened this issue May 9, 2017 · 7 comments
Open
1 of 2 tasks

Refactor & review test #259

lukasoppermann opened this issue May 9, 2017 · 7 comments
Labels
good first issue help wanted

Comments

@lukasoppermann
Copy link
Owner

@lukasoppermann lukasoppermann commented May 9, 2017

  • Tests need to be split into smaller, more meaningful files
  • Tests needs to be reviews, what is missing, what is actually tested
@lukasoppermann lukasoppermann added good first issue help wanted labels May 9, 2017
@lukasoppermann lukasoppermann added this to Roadmap in current Version Feb 22, 2018
@lukasoppermann lukasoppermann changed the title Cleanup test Refactor & review test Feb 22, 2018
@jMuzsik
Copy link
Contributor

@jMuzsik jMuzsik commented Mar 3, 2018

Hi! I'd be willing to work on this

@lukasoppermann
Copy link
Owner Author

@lukasoppermann lukasoppermann commented Mar 4, 2018

Hey @jMuzsik, very awesome! I just merged #343 which changed the framework to jest.

I did refactor some tests for units I did extract e.g. debounce but the big part is still in the files:

  • tests/api.test.ts
  • tests/events.test.ts
  • tests/internal.test.ts
  • tests/ghost.test.ts

I think they should all be split up into smaller files which only test specific things. __tests__/internal.test.ts should in the long run be completely removed and replaced with tests like the debounce.test.ts.

Looking forward to your help. It would be great if you could create a PR immediatly when you push your first branch and just name it with 🛑 [WIP] in the beginning so that I know it is not ready to be merged. This way I can see what you are working on so that I don't work on the same. 😉

Also if you can package your changes into small PRs that would be best, as I can merge them faster and easier. 👍

Just FYI, as I don't know how well you know jest:

  • npm test runs all files in __tests__ that are named xyz.test.ts
  • npm run jest runs jest so you can do npm run jest something and it will run __tests__/something.test.ts
  • npm run coverage shows you the test coverage

Thanks mate.

@lukasoppermann lukasoppermann moved this from Roadmap to In Progress in current Version Mar 4, 2018
@jMuzsik
Copy link
Contributor

@jMuzsik jMuzsik commented Mar 5, 2018

Ok! Great, so for example: in Internal function tests:

_removeSortableEvents
_removeItemEvents
_removeItemData
...all the rest of the functions

all ought to be in their own files, and I will need to slightly modify the code to fit into the Jest framework just as you did with debounce.

Simultaneously, I ought to put more descriptive comments about what is exactly happening within the test? Though, solely if the comments are not already there. (this I cannot guarantee ideal work as I cannot assure that my reading/testing of the code will be the truth).

Or am I reading this wrong: " Tests needs to be reviews, what is missing, what is actually tested
". Is it more nuanced than I thought?

I'm working on several things right now as well, so I'll likely be doing a little every few days if that is alright.

@lukasoppermann
Copy link
Owner Author

@lukasoppermann lukasoppermann commented Mar 5, 2018

Hey @jMuzsik,

I'm working on several things right now as well, so I'll likely be doing a little every few days if that is alright.

Sure, I appreciate any help you can put it, no worries.

Adding descriptive comments will definitely be very helpful. Feel free to also improve comments that are already there.

And yes, refactoring into smaller files, like your example suggests would be great!
Feel free to modify the testing code as much as you think is good: The tests are fairly old and might not be written very well or might be missing some cases. So feel free to change, update, etc. whatever you think makes sense.

For example, in many cases we can probably just use createElement instead of jsdom. (Like in index.test.ts).

If you find issues / improvements with the package code, please feel free to send PRs for this or create issues. 😉

And if you don't understand something, let me know so I can help (some of the code is written in a rather complicated way, which I am hoping to improve soon).

Thank you very much. 👍

@lukasoppermann
Copy link
Owner Author

@lukasoppermann lukasoppermann commented Mar 5, 2018

Also, I only added jsdom because I copied the old tests. Jest has jsdom included by default, so you can just do it like in isInDom.test.ts (sorry if you already know this).

@jMuzsik
Copy link
Contributor

@jMuzsik jMuzsik commented Mar 5, 2018

Ok got it! I'll begin working on this today.

@kaffarell
Copy link
Collaborator

@kaffarell kaffarell commented Jul 19, 2020

@jMuzsik is this already finished or are you still working on it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue help wanted
Projects
current Version
  
In Progress
Development

No branches or pull requests

3 participants