Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rework tests from gocheck to stdlib testing #17

Merged
merged 1 commit into from
Jul 17, 2018
Merged

rework tests from gocheck to stdlib testing #17

merged 1 commit into from
Jul 17, 2018

Conversation

acln0
Copy link
Member

@acln0 acln0 commented Jul 15, 2018

Use the standard library testing package instead of gocheck. Switch
from multiple assertions to table-driven tests where appropriate.

Update fuzz testing support: add code to generate a diverse corpus
based on existing parser tests, add instructions on how to run the
fuzzer, check in the existing corpus.

@acln0 acln0 requested a review from theckman July 15, 2018 04:36
@coveralls
Copy link

Pull Request Test Coverage Report for Build 50

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 98.442%

Totals Coverage Status
Change from base Build 39: 0.3%
Covered Lines: 316
Relevant Lines: 321

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jul 15, 2018

Pull Request Test Coverage Report for Build 70

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 98.442%

Totals Coverage Status
Change from base Build 68: 0.3%
Covered Lines: 316
Relevant Lines: 321

💛 - Coveralls

@coveralls
Copy link

Pull Request Test Coverage Report for Build 50

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 98.442%

Totals Coverage Status
Change from base Build 39: 0.3%
Covered Lines: 316
Relevant Lines: 321

💛 - Coveralls

@acln0
Copy link
Member Author

acln0 commented Jul 15, 2018

There are probably things that I missed here. I'm sending this PR for preliminary review. It's fairly big, and it's going to need some eyes on it before a potential merge.

@acln0
Copy link
Member Author

acln0 commented Jul 15, 2018

Ugh. I completely forgot Go < 1.7 doesn't have subtests. Travis isn't happy. What should we do? I can get rid of all the subtests if we need to.

@theckman
Copy link
Member

@acln0 I wouldn't be against dropping the test runners for the older Go versions within the context of this PR.

Outside of this PR, we should have the discussion as an organization as to whether our support of Go runtime versions differs from the authors.

@acln0
Copy link
Member Author

acln0 commented Jul 15, 2018

Filed #18 for discussion.

@acln0 acln0 mentioned this pull request Jul 15, 2018
theckman added a commit that referenced this pull request Jul 16, 2018
In preparation for executing on #15 (already opened in #17) we need to disable
testing against older versions of the Go toolchain. There are a few reasons for
this, the largest being they don't support subtests. These versions, including
Go 1.7 and 1.8, are EOL by the Go authors so there's also lack of upstream
support for these toolchains.

This was discussed a bit in #18 and on
[Slack](https://gophers.slack.com/archives/CBP4N9BEU/p1531704009000014).

Fixes #18

Signed-off-by: Tim Heckman <t@heckman.io>
theckman added a commit that referenced this pull request Jul 16, 2018
In preparation for executing on #15 (already opened in #17) we need to disable
testing against older versions of the Go toolchain. There are a few reasons for
this, the largest being they don't support subtests. These versions, including
Go 1.7 and 1.8, are EOL by the Go authors so there's also lack of upstream
support for these toolchains.

This was discussed a bit in #18 and on
[Slack](https://gophers.slack.com/archives/CBP4N9BEU/p1531704009000014).

Fixes #18

Signed-off-by: Tim Heckman <t@heckman.io>
theckman added a commit that referenced this pull request Jul 16, 2018
In preparation for executing on #15 (already opened in #17) we need to disable
testing against older versions of the Go toolchain. There are a few reasons for
this, the largest being they don't support subtests. These versions, including
Go 1.7 and 1.8, are EOL by the Go authors so there's also lack of upstream
support for these toolchains.

This was discussed a bit in #18 and on
[Slack](https://gophers.slack.com/archives/CBP4N9BEU/p1531704009000014).

Fixes #18

Signed-off-by: Tim Heckman <t@heckman.io>
theckman added a commit that referenced this pull request Jul 16, 2018
In preparation for executing on #15 (already opened in #17) we need to disable
testing against older versions of the Go toolchain. There are a few reasons for
this, the largest being they don't support subtests. These versions, including
Go 1.7 and 1.8, are EOL by the Go authors so there's also lack of upstream
support for these toolchains.

This was discussed a bit in #18 and on
[Slack](https://gophers.slack.com/archives/CBP4N9BEU/p1531704009000014).

Fixes #18

Signed-off-by: Tim Heckman <t@heckman.io>
theckman added a commit that referenced this pull request Jul 16, 2018
In preparation for executing on #15 (already opened in #17) we need to disable
testing against older versions of the Go toolchain. There are a few reasons for
this, the largest being they don't support subtests. These versions, including
Go 1.7 and 1.8, are EOL by the Go authors so there's also lack of upstream
support for these toolchains.

This was discussed a bit in #18 and on [Slack](https://gophers.slack.com/archives/CBP4N9BEU/p1531704009000014).

Fixes #18

Signed-off-by: Tim Heckman <t@heckman.io>
@acln0 acln0 requested a review from niaow July 16, 2018 08:13
@theckman
Copy link
Member

@acln0 can you rebase this branch against master and then force-push it back up? That should get this to be green I think.

Copy link
Member

@niaow niaow left a comment

Choose a reason for hiding this comment

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

Aside from that, looks good!

Copy link
Member

@niaow niaow left a comment

Choose a reason for hiding this comment

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

This looks good!

Use the standard library testing package instead of gocheck. Switch
from multiple assertions to table-driven tests where appropriate.

Update fuzz testing support: add code to generate a diverse corpus
based on existing parser tests, add instructions on how to run the
fuzzer, check in the existing corpus.
@acln0
Copy link
Member Author

acln0 commented Jul 17, 2018

@theckman Done.

@jadr2ddude Yes, that works, because the Variant method only touches that single byte.

@theckman
Copy link
Member

@acln0 awesome! I did a quick pass on this PR before, but let me give it a look again tonight. It'll be a few hours until I'm at home and able to give it some proper 👀.

@acln0
Copy link
Member Author

acln0 commented Jul 17, 2018

Thanks. I forgot to mention it in the commit message, but, once merged, this closes #2 and #15.

@theckman theckman merged commit 57be146 into gofrs:master Jul 17, 2018
theckman added a commit that referenced this pull request Jul 17, 2018
rework tests from gocheck to stdlib testing

Fixes #2
Fixes #15

Signed-off-by: Tim Heckman <t@heckman.io>
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.

None yet

4 participants