Skip to content
This repository has been archived by the owner on Jul 5, 2024. It is now read-only.

Printer: add tests #126

Merged
merged 8 commits into from
Mar 16, 2020
Merged

Conversation

isacikgoz
Copy link
Member

Summary

Add tests to printer package, a humble start for refactoring.

Ticket Link

No tickets yet.

@hanzei hanzei added the 2: Dev Review Requires review by a core committer label Feb 29, 2020
@hanzei
Copy link
Contributor

hanzei commented Mar 11, 2020

/update-branch

@codecov-io
Copy link

codecov-io commented Mar 11, 2020

Codecov Report

Merging #126 into master will increase coverage by 0.41%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
+ Coverage   60.73%   61.15%   +0.41%     
==========================================
  Files          27       28       +1     
  Lines        2045     2085      +40     
==========================================
+ Hits         1242     1275      +33     
- Misses        763      768       +5     
- Partials       40       42       +2     
Impacted Files Coverage Δ
printer/printer.go 82.50% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6d06580...84cfea8. Read the comment docs.

Copy link
Member

@mgdelacroix mgdelacroix left a comment

Choose a reason for hiding this comment

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

I like the approach a lot, thanks for working on this @isacikgoz! A couple of minor comments noted

printer/printer_test.go Outdated Show resolved Hide resolved
printer/printer_test.go Outdated Show resolved Hide resolved
Print("test string")
Flush()

assert.Equal(t, "[\n \"test string\"\n]\n", string(mw.buffer))
Copy link
Member

Choose a reason for hiding this comment

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

Can we add to the assertions a check for the state of printer.Lines?

Print("test string-1")
Print("test string-2")
Flush()
assert.Equal(t, "[\n \"test string-1\",\n \"test string-2\"\n]\n", string(mw.buffer))
Copy link
Member

Choose a reason for hiding this comment

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

Same check for printer.Lines here

isacikgoz and others added 2 commits March 13, 2020 11:52
Co-Authored-By: Miguel de la Cruz <[email protected]>
Co-Authored-By: Miguel de la Cruz <[email protected]>
@mgdelacroix mgdelacroix self-requested a review March 14, 2020 16:55
printer/printer_test.go Show resolved Hide resolved
printer/printer_test.go Show resolved Hide resolved
Copy link
Member

@mgdelacroix mgdelacroix left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!! LGTM 👍

@mgdelacroix mgdelacroix added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core committer labels Mar 16, 2020
@mgdelacroix mgdelacroix merged commit cef1dc6 into mattermost:master Mar 16, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants