Skip to content

Add tests to four methods in table.go#5

Closed
aimof wants to merge 3 commits intorivo:masterfrom
aimof:test-table
Closed

Add tests to four methods in table.go#5
aimof wants to merge 3 commits intorivo:masterfrom
aimof:test-table

Conversation

@aimof
Copy link
Copy Markdown

@aimof aimof commented Jan 10, 2018

Hi.

I add tests to four methods in table.go.

Thanks.

@rivo
Copy link
Copy Markdown
Owner

rivo commented Jan 10, 2018

Hi,

I appreciate your contribution. I'm planning on adding tests when we get closer to a v1.0 release because there may still be slight changes pre v1.0. Especially when testing the default values set in constructors, your tests may become obsolete quickly. I'm also not sure if we should have tests for simple setters and getters. In TestSetBorders() you are adding 20 lines of test code for effectively one line of actual code: t.borders = show. My question is, will you be willing to adapt your test cases if I make changes to my functions? (The package is only a few days old, there will likely be some changes.)

What I'm looking for are test cases where a primitive is drawn, some keyboard input is simulated, and the output is compared with the expected screen output. I'm still thinking about this.

More specific feedback to your code:

  • Please don't use tcell.Color values in your tests. There is now a new Style variable with default colors. So line 44 should be Styles.BorderColor instead.
  • Could you please name your test functions Test<type name><func name>? For example, TestTableSetBorders() instead of TestSetBorders(). I cannot rule out that there will be functions with the same name in other components. (TestNew<type name> should be ok.)
  • Please remove the newlines at the end of t.Error() calls. I'm not sure why you added them but I think they are not needed.

Thanks.

@aimof
Copy link
Copy Markdown
Author

aimof commented Jan 11, 2018

Thanks to your kind feedbuck.

I understand my PR is not good one.
If you think the PR is not needed now, please close this PR.
Else, i rewrite this PR.

I appreciate your polite response and feedbuck.

Thanks.

@rivo
Copy link
Copy Markdown
Owner

rivo commented Jan 11, 2018

Hi,

Your code is generally ok and I appreciate your effort but I'm afraid it's a bit too early so I will close it for now. I may get back to this at a later point. But if you want to contribute, I'm happy to prepare some functions for the test cases I envision.

Let me know if you're interested and I'll have something ready for you in the next few days.

Cheers!

@rivo rivo closed this Jan 11, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants