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.

Sign up for GitHub

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

Jump to bottom

Fix flytectlt tests #5325

Merged
merged 10 commits into from
May 3, 2024
Merged

Fix flytectlt tests #5325

merged 10 commits into from
May 3, 2024

Conversation

eapolinario
Copy link
Contributor

Tracking issue

#5316

Why are the changes needed?

We fixed a few of the flytectl issues in #5309, but decided to merge that PR with a few unit tests failing. This PR fixes them.

What changes were proposed in this pull request?

We have a few flytectl unit tests that rely on a TestStruct to capture what's written to standard output and error, but if those file descriptors are not restored then the respective test suites would fail every now and then. In this PR we add a defer statement to ensure that the original file descriptors are restored to the os package.

How was this patch tested?

Unit tests running in a loop locally (e.g. seq 100 | xargs -n1 make test_unit).

Setup process

Screenshots

Check all the applicable boxes

  • I updated the documentation accordingly.
  • All new and existing tests passed.
  • All commits are signed-off.

Related PRs

Docs link

@eapolinario
Add defer to compile_test
6f5778f 
Signed-off-by: Eduardo Apolinario <[email protected]>
@eapolinario
Add the actual command
b17fbf1 
Signed-off-by: Eduardo Apolinario <[email protected]>
@eapolinario
Fix cmd/create tests
3490bfa 
Signed-off-by: Eduardo Apolinario <[email protected]>
@eapolinario
Fix create tests
6e212fb 
Signed-off-by: Eduardo Apolinario <[email protected]>
@eapolinario
Fix cmd/get tests
c2cfb37 
Signed-off-by: Eduardo Apolinario <[email protected]>
@eapolinario
Fix cmd/update tests
a3c9e02 
Signed-off-by: Eduardo Apolinario <[email protected]>
@eapolinario
Remove testutils.SetupWithExt
81b9352 
Signed-off-by: Eduardo Apolinario <[email protected]>
@eapolinario
Remove bespoke test_unit make target
75b9a33 
Signed-off-by: Eduardo Apolinario <[email protected]>
@eapolinario
Fix other tests
c061933 
Signed-off-by: Eduardo Apolinario <[email protected]>
eapolinario
eapolinario commented May 3, 2024
View reviewed changes
flytectl/cmd/testutils/test_utils.go
Comment on lines -73 to -98
func SetupWithExt() (s TestStruct) {
s.Ctx = context.Background()
s.Reader, s.Writer, s.Err = os.Pipe()
if s.Err != nil {
panic(s.Err)
}
s.StdOut = os.Stdout
s.Stderr = os.Stderr
os.Stdout = s.Writer
os.Stderr = s.Writer
log.SetOutput(s.Writer)
s.MockClient = admin.InitializeMockClientset()
s.FetcherExt = new(extMocks.AdminFetcherExtInterface)
s.UpdaterExt = new(extMocks.AdminUpdaterExtInterface)
s.DeleterExt = new(extMocks.AdminDeleterExtInterface)
s.FetcherExt.OnAdminServiceClient().Return(s.MockClient.AdminClient())
s.UpdaterExt.OnAdminServiceClient().Return(s.MockClient.AdminClient())
s.DeleterExt.OnAdminServiceClient().Return(s.MockClient.AdminClient())
s.MockAdminClient = s.MockClient.AdminClient().(*mocks.AdminServiceClient)
s.MockOutStream = s.Writer
s.CmdCtx = cmdCore.NewCommandContextWithExt(s.MockClient, s.FetcherExt, s.UpdaterExt, s.DeleterExt, s.MockOutStream)
config.GetConfig().Project = projectValue
config.GetConfig().Domain = domainValue
config.GetConfig().Output = output

return s
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This function is an exact copy of Setup, so I'm dropping it.

@codecov Codecov
Copy link

codecov bot commented May 3, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 61.07%. Comparing base (8cc9617) to head (8fb8a99).

Additional details and impacted files 
@@            Coverage Diff             @@
##           master    #5325      +/-   ##
==========================================
+ Coverage   60.20%   61.07%   +0.86%     
==========================================
  Files         646      794     +148     
  Lines       45654    51203    +5549     
==========================================
+ Hits        27487    31270    +3783     
- Misses      15575    17050    +1475     
- Partials     2592     2883     +291
Flag Coverage Δ
unittests-datacatalog 69.31% <ø> (ø)
unittests-flyteadmin 58.78% <ø> (-0.05%) ⬇️
unittests-flytecopilot 17.79% <ø> (ø)
unittests-flytectl 68.30% <ø> (?)
unittests-flyteidl 79.30% <ø> (ø)
unittests-flyteplugins 61.94% <ø> (ø)
unittests-flytepropeller 57.32% <ø> (ø)
unittests-flytestdlib 65.73% <ø> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

katrogan
katrogan previously approved these changes May 3, 2024
View reviewed changes
Copy link
Contributor

@katrogan katrogan left a comment

Choose a reason for hiding this comment

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

nice find

flytectl/cmd/create/execution_util_test.go Outdated
@@ -48,6 +48,8 @@ func createExecutionUtilSetup() {

func TestCreateExecutionForRelaunch(t *testing.T) {
s := setup()
defer s.RestoreStandardFileDescriptors()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: maybe call this teardown() to match with go test name conventions, so users don't have to think about whether or not to call this function (and potentially miss doing so?)

@eapolinario eapolinario dismissed stale reviews from katrogan and pmahindrakar-oss via 8fb8a99 May 3, 2024 21:46
@eapolinario eapolinario enabled auto-merge (squash) May 3, 2024 22:00
@eapolinario eapolinario merged commit 8dd8b05 into master May 3, 2024
50 of 51 checks passed
@eapolinario eapolinario deleted the fix-flytectlt-tests branch May 3, 2024 22:02
austin362667 pushed a commit to austin362667/flyte that referenced this pull request May 7, 2024
@eapolinario @austin362667
Fix flytectlt tests (flyteorg#5325)
3d5fd82 
* Add defer to compile_test

Signed-off-by: Eduardo Apolinario <[email protected]>

* Add the actual command

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix cmd/create tests

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix create tests

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix cmd/get tests

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix cmd/update tests

Signed-off-by: Eduardo Apolinario <[email protected]>

* Remove testutils.SetupWithExt

Signed-off-by: Eduardo Apolinario <[email protected]>

* Remove bespoke `test_unit` make target

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix other tests

Signed-off-by: Eduardo Apolinario <[email protected]>

* s/RestoreStandardFileDescriptors/TearDown/g

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
@eapolinario eapolinario mentioned this pull request Jul 2, 2024
Merged
3 tasks
robert-ulbrich-mercedes-benz pushed a commit to robert-ulbrich-mercedes-benz/flyte that referenced this pull request Jul 2, 2024
@eapolinario @robert-ulbrich-mercedes-benz
Fix flytectlt tests (flyteorg#5325)
8a87572 
* Add defer to compile_test

Signed-off-by: Eduardo Apolinario <[email protected]>

* Add the actual command

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix cmd/create tests

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix create tests

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix cmd/get tests

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix cmd/update tests

Signed-off-by: Eduardo Apolinario <[email protected]>

* Remove testutils.SetupWithExt

Signed-off-by: Eduardo Apolinario <[email protected]>

* Remove bespoke `test_unit` make target

Signed-off-by: Eduardo Apolinario <[email protected]>

* Fix other tests

Signed-off-by: Eduardo Apolinario <[email protected]>

* s/RestoreStandardFileDescriptors/TearDown/g

Signed-off-by: Eduardo Apolinario <[email protected]>

---------

Signed-off-by: Eduardo Apolinario <[email protected]>
Co-authored-by: Eduardo Apolinario <[email protected]>
This was referenced Nov 1, 2024
Merged
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@katrogan katrogan katrogan approved these changes

@pmahindrakar-oss pmahindrakar-oss pmahindrakar-oss left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eapolinario @katrogan @pmahindrakar-oss