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

Bulk convert to using testify/suite for tests #1185

Merged
merged 5 commits into from
May 30, 2024
Merged

Conversation

ashb
Copy link
Contributor

@ashb ashb commented May 8, 2023

This is in some regards a "change for no purpose" but it lets us more
easily add setup or teardown to tests.

The go test compatible test was named after the Go packge
./pkg/workspace becomes TestPkgWorkspaceSuite.

This change was almost entirely automatically applied using
github.com/uber-go/gopatch

I have not converted 100% of tests to use suites, some of them proved to have ordering bugs (tests relying on state left over or setup from a previous test) that I wasn't able to easily/quickly solve, and I haven't applied Setup/Teardown functions universally, only where I specifically noticed them.

The patch file used was this:

@@
var foo identifier
@@
 import "testing"

-	t.Errorf(...)
+ s.Fail(...)

@@
@@
 import "github.com/stretchr/testify/assert"

-if err != nil {
-	t.Error(err)
-}
+assert.NoError(t, err)

@@
@@
 import "github.com/stretchr/testify/assert"

-assert.Nil(t, err)
+assert.NoError(t, err)

@@
var foo identifier
@@
-func foo(t *testing.T) {
+func (s *Suite) foo(t *testing.T) {
  ...
 }

@@
var name expression
@@
-t.Run(name, func(t *testing.T) {
+s.Run(name, func() {
  ...
 })

@@
var bar identifier
@@
 import "github.com/stretchr/testify/assert"

- assert.bar(t, ...)
+ s.bar(...)

@@
@@

-if err != nil {
-	t.Fatal(err)
-}
+s.Require().NoError(err)

@@
var foo identifier
@@
-func (s *Suite) foo(t *testing.T) {
+func (s *Suite) foo() {
  ...
 }

@@
var foo identifier
@@
-foo.AssertExpectations(t)
+foo.AssertExpectations(s.T())

# Need to change this one back
@@
@@
-    t.Cleanup(func() { mock.AssertExpectations(s.T()) })
+    t.Cleanup(func() { mock.AssertExpectations(t) })

@@
@@
import testUtil "github.com/astronomer/astro-cli/pkg/testing"

-    testUtil.MockUserInput(t, ...)
+    testUtil.MockUserInput(s.T(), ...)


# Reset the suite test back to taking testing.T
@@
var foo identifier
@@
import (
  "testing"
	"github.com/stretchr/testify/suite"
)

-func (s *Suite) foo() {
+func foo(t *testing.T) {
	  suite.Run(t, new(Suite))
 }


@@
var tt identifier
var foo identifier
@@
import (
  "testing"
	"github.com/stretchr/testify/assert"
)

func (s *Suite) foo() {
- tt.errAsseration(t, ...)
+ tt.errAsseration(s.T(), ...)
}

@kushalmalani
Copy link
Contributor

@ashb This PR has been in draft state for a very long time? Do you still plan to proceed further?

ashb added 3 commits May 29, 2024 10:25
There were a couple of places where `cmdExec` was changed but not
changed back.

Now by this being a test suit it's handled for us
And also notice that the test files were misnamed so not always run!
@ashb ashb marked this pull request as ready for review May 29, 2024 09:51
@ashb
Copy link
Contributor Author

ashb commented May 29, 2024

I've just updated this PR and it's ready for review now.

@ashb
Copy link
Contributor Author

ashb commented May 29, 2024

Tests not updated by this PR

 M cloud/auth/auth_test.go
 M cloud/deploy/deploy_test.go
 M cloud/deployment/deployment_variable_test.go
 M cloud/deployment/inspect/inspect_test.go
 M cloud/user/user_test.go
 M cmd/airflow_test.go
 M cmd/cloud/deploy_test.go
 M cmd/cloud/deployment_inspect_test.go
 M cmd/cloud/deployment_objects_test.go
 M cmd/cloud/deployment_test.go
 M cmd/cloud/deployment_workerqueue_test.go
 M cmd/cloud/organization_test.go
 M cmd/cloud/root_test.go
 M cmd/cloud/setup_test.go
 M cmd/cloud/workspace_test.go
 M cmd/root_test.go
 M cmd/software/root_test.go
 M cmd/utils/utils_test.go
 M pkg/git/git_test.go

Copy link
Contributor

@neel-astro neel-astro left a comment

Choose a reason for hiding this comment

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

I have breezed through all the file changes, it's hard to stop and check each line change. But overall LGTM. Also, we should aim to solve the issue with the shared state between tests, happy to help out with that effort @kushalmalani.

@ashb
Copy link
Contributor Author

ashb commented May 29, 2024

Yeah, almost all of these changes were automatic by the patch in the PR desc, or manually creating the driver test:

type Suite struct {
	suite.Suite
}

func TestAirflowClient(t *testing.T) {
	suite.Run(t, new(Suite))
}

or in a few cases fixing left over state from one test to another etc.

@ashb ashb force-pushed the switch-testify-suites branch 3 times, most recently from 45794d0 to 420a6b8 Compare May 29, 2024 13:24
Copy link

codecov bot commented May 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.65%. Comparing base (d858097) to head (fc17cc1).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1185      +/-   ##
==========================================
- Coverage   86.72%   86.65%   -0.07%     
==========================================
  Files         114      114              
  Lines       16707    16709       +2     
==========================================
- Hits        14489    14480       -9     
- Misses       1329     1337       +8     
- Partials      889      892       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ashb
Copy link
Contributor Author

ashb commented May 29, 2024

Is this saying it things the coverage has dropped by 22% 😱

Yeah, I've messed something up. On main:

$ go test -coverprofile=coverage-main.txt -covermode atomic ./airflow/
ok      github.com/astronomer/astro-cli/airflow 9.826s  coverage: 88.9% of statements
$ go test -coverprofile=coverage.txt -covermode atomic ./airflow/
ok      github.com/astronomer/astro-cli/airflow 6.218s  coverage: 55.6% of statements

Whoops foolish Ash, I disabled a bunch of tests in my testing

@ashb
Copy link
Contributor Author

ashb commented May 29, 2024

Okay I fixed that issue, but coverage is still reporting a big drop, but the individual views aren't showing what's actually dropped.

Copy link
Contributor

@kushalmalani kushalmalani 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 and much needed to improve testing framework in CLI. Thanks ash!

@ashb ashb merged commit fc96bb3 into main May 30, 2024
4 of 5 checks passed
@ashb ashb deleted the switch-testify-suites branch May 30, 2024 19:26
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

3 participants