ghiscoding / slickgrid-universal

Slickgrid-Universal is a monorepo which includes all Editors, Filters, Extensions, Services related to SlickGrid usage and is also Framework Agnostic
https://ghiscoding.github.io/slickgrid-universal/
Other
90 stars 29 forks source link

chore: add SlickCellExternalCopyManager test coverage #1666

Closed ghiscoding closed 2 months ago

ghiscoding commented 2 months ago

@zewa666 oops it looks like I merged the PR a little too early, the code below is not being covered. I did add coverage for the first line but the condition below is not covered. It actually looks like it's not even possible to go into the if condition, because the addRows variable is assigned to 0 and so how can it even go into the if condition?

is the code shown below still needed?

image

stackblitz[bot] commented 2 months ago

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

codecov[bot] commented 2 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 99.8%. Comparing base (d8403ea) to head (3afa0e7). Report is 4 commits behind head on master.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #1666 +/- ## ======================================== + Coverage 99.7% 99.8% +0.1% ======================================== Files 187 187 Lines 31085 31076 -9 Branches 9782 9782 ======================================== Hits 30987 30987 + Misses 98 89 -9 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

zewa666 commented 2 months ago

hmmm havent seen that line. where is that code from? also do I need to do anything specific to get the new tests to run or merely pnpm install/bundle and than run test again?

ghiscoding commented 2 months ago

@zewa666 I did some more research and the code came from the original SlickGrid but was no longer covered after your PR because you removed the newRowCreator which was the only place that was incrementing the addRows variable. But now that the creator is remove, this code is totally unreachable (addRows is always set to 9 and so the if condition is always false).

Here's the code before your PR removed

https://github.com/ghiscoding/slickgrid-universal/blob/7b485962024a4ce336ebe80d223ddba4b7e80824/packages/common/src/extensions/slickCellExternalCopyManager.ts#L281-L289)

and here's your PR changes

image

so should I just get rid of code since it's unreachable or does that previous for loop still had something that we should add back?

also do I need to do anything specific to get the new tests to run or merely pnpm install/bundle and than run test again?

just pnpm install and run same scripts as before, the only thing that changed is the Debugger name which is now "Vitest - Debug Current Test File"

zewa666 commented 2 months ago

thats really weird, as far as I get the code was merely dropping and re-adding the same rows. is that to cause a rerender on a fresh new array instance?

I've tried my fix first locally and so far we havent noticed any issues but who knows what this was for. I'll check that out once more but from a quick first look i'd say off it goes that unused code 😅

ghiscoding commented 2 months ago

yeah sure, let me remove it and you'll have more chances to test it over in the coming weeks so if something is wrong I'm sure you'll provide a PR because like I said before, I'm not really using this plugin myself. 😉

zewa666 commented 2 months ago

copy and paste is so huge for my company as our tool is here to replace all these renegade excel files. so naturally people try to copy and paste the hell back and forth.

Imagine how awesome it is to debug issues there, like the last one where randomly 2 cols werent pasted but the others were. just to find out that the excel had 3 merged cells, the copy buffer though still contains the individual cols and therefore tabs 🤣