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
all: rewrite tests to use testify instead of goconvey #300
Conversation
+1 |
@unknwon Please let me know if you think this is the right path. If so, I can finish the PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for taking on this effort! LGTM
@unknwon I finished the PR, please take a look. |
@unknwon Ping :) Would be nice to get this merged before other PRs start conflicting with it....it's a pretty big diff. |
Codecov Report
@@ Coverage Diff @@
## main #300 +/- ##
=======================================
Coverage 88.11% 88.11%
=======================================
Files 9 9
Lines 1355 1355
=======================================
Hits 1194 1194
Misses 98 98
Partials 63 63 |
file_test.go
Outdated
"github.com/stretchr/testify/require" | ||
"gopkg.in/ini.v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"github.com/stretchr/testify/require" | |
"gopkg.in/ini.v1" | |
"github.com/stretchr/testify/require" | |
"gopkg.in/ini.v1" |
ini_python_multiline_test.go
Outdated
"github.com/stretchr/testify/require" | ||
"gopkg.in/ini.v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"github.com/stretchr/testify/require" | |
"gopkg.in/ini.v1" | |
"github.com/stretchr/testify/require" | |
"gopkg.in/ini.v1" |
ini_test.go
Outdated
"github.com/stretchr/testify/require" | ||
"gopkg.in/ini.v1" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"github.com/stretchr/testify/require" | |
"gopkg.in/ini.v1" | |
"github.com/stretchr/testify/require" | |
"gopkg.in/ini.v1" |
ini_test.go
Outdated
So(err, ShouldBeNil) | ||
So(f, ShouldNotBeNil) | ||
require.NoError(t, err) | ||
assert.NotNil(t, f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.NotNil(t, f) | |
require.NotNil(t, f) |
ini_test.go
Outdated
So(err, ShouldBeNil) | ||
So(f, ShouldNotBeNil) | ||
require.NoError(t, err) | ||
assert.NotNil(t, f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.NotNil(t, f) | |
require.NotNil(t, f) |
ini_test.go
Outdated
t.Run("inverse case", func(t *testing.T) { | ||
f, err := ini.LoadSources(ini.LoadOptions{AllowPythonMultilineValues: true}, minimalConf) | ||
require.NoError(t, err) | ||
assert.NotNil(t, f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.NotNil(t, f) | |
require.NotNil(t, f) |
ini_test.go
Outdated
t.Run("insensitive to sections and sensitive to key names", func(t *testing.T) { | ||
f, err := ini.LoadSources(ini.LoadOptions{InsensitiveSections: true}, minimalConf) | ||
require.NoError(t, err) | ||
assert.NotNil(t, f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.NotNil(t, f) | |
require.NotNil(t, f) |
ini_test.go
Outdated
t.Run("sensitive to sections and insensitive to key names", func(t *testing.T) { | ||
f, err := ini.LoadSources(ini.LoadOptions{InsensitiveKeys: true}, minimalConf) | ||
require.NoError(t, err) | ||
assert.NotNil(t, f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.NotNil(t, f) | |
require.NotNil(t, f) |
ini_test.go
Outdated
t.Run("inverse case", func(t *testing.T) { | ||
f, err := ini.LoadSources(ini.LoadOptions{}, minimalConf) | ||
require.NoError(t, err) | ||
assert.NotNil(t, f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
assert.NotNil(t, f) | |
require.NotNil(t, f) |
ini_test.go
Outdated
So(err, ShouldBeNil) | ||
So(f, ShouldNotBeNil) | ||
require.NoError(t, err) | ||
assert.NotNil(t, f) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as every other place.
Thanks, lint is failing BTW. |
Signed-off-by: Mark Sagi-Kazar <mark.sagikazar@gmail.com>
Thanks for the review @unknwon ! Should be good now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
Breaking lint job seems unrelated. I think it's mostly because go modules (or lack thereof). Happy to take a look at it in a followup PR. |
There is one valid lint error
but the golangci GitHub Action is playing smart to not tell us which file it is. |
OK, looks like that's a file somewhere on |
Do you think you could tag a new version as well? |
Here you go: https://github.com/go-ini/ini/releases/tag/v1.63.1 |
Thank you! |
What problem should be fixed?
Fixes #295
Have you added test cases to catch the problem?
This PR is about refactoring tests, no additional tests are necessary.
Notes for reviewer
This is an attempt to refactor tests into using testify instead of goconvey.
During the refactor I followed the following rules:
Convey
was removed everywhereConvey
blocks were converted into subtests (t.Run
)Convey
blocks were converted into subtests (t.Run
)require
was used everywhere when the following assertions depended on the outcome (eg. no error or no nil return value)assert
was used everywhere elseI suggest turning off showing whitespaces in the diff because there are lots of indentation changes.
If you like the result, I can refactor the remaining tests as well.