Skip to content
This repository has been archived by the owner on Jun 26, 2020. It is now read-only.

Restored a part of code and added a test for it #108

Merged
merged 2 commits into from
Apr 21, 2020
Merged

Restored a part of code and added a test for it #108

merged 2 commits into from
Apr 21, 2020

Conversation

pomek
Copy link
Member

@pomek pomek commented Jan 14, 2020

Suggested merge commit message (convention)

Tests: Restored a condition that caused an error in test logs in the @ckeditor/ckeditor5-image package. Added a test to cover the code and make 100% CC again. Closes ckeditor/ckeditor5#5892.

@pomek pomek requested a review from Reinmar January 14, 2020 06:30
@coveralls
Copy link

coveralls commented Jan 14, 2020

Coverage Status

Coverage remained the same at 100.0% when pulling 2e29e9a on i/5892 into 64209ec on master.

@Reinmar
Copy link
Member

Reinmar commented Jan 28, 2020

You shouldn't just copy an image test to this package. Image is a consumer of this package and has its own tests. This package should has its own, generic tests. You should check how the image test triggers this single case and transform this to a generic test for this package. One which will not create dependencies on this package's consumer (image became this package's dependency and this is already fishy).

Copy link
Member

@Reinmar Reinmar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As commented.

@pomek
Copy link
Member Author

pomek commented Apr 20, 2020

Because CI does not work for a while, I'm pasting screenshots from CC report:

image

image

@Reinmar, ready for review once again.

@Reinmar Reinmar merged commit ed7187b into master Apr 21, 2020
@Reinmar Reinmar deleted the i/5892 branch April 21, 2020 13:12
@Reinmar
Copy link
Member

Reinmar commented Apr 21, 2020

I closed this as a "Fix" because it actually resolves a bug that we introduced by removing this condition. This bug was manifested in tests only, but it may also perhaps somehow appear irl.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve the CC in the Upload package
3 participants