Refactor & review test

[lukasoppermann]

• Tests need to be split into smaller, more meaningful files
• Tests needs to be reviews, what is missing, what is actually tested

[jmuzsik]

Hi! I'd be willing to work on this

[lukasoppermann]

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.

[jmuzsik]

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]

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]

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]

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

[kaffarell]

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

[lukasoppermann]

If anyone is still interested, this is still ongoing.

[tridungng]

@lukasoppermann Hey is this issue still ongoing? If it is the case I would like a short guidance and will try to work on it

[lukasoppermann]

Hey @tridungng, yes it is.
This is basically still true: #259 (comment)

There is no open PR, so you can just create a new PR for your work. I would appreciate it if you could create small PRs. E.g. fix one or two tests and send a PR with just those. It makes it easier for me to review and merge.

If you have any specific questions, please let me know so I can provide better guidance.