Fixed suite TearDown ordering #799
Conversation
(cherry picked from commit 92c097a5af7b4bbbe3a66c2e7484f68194cd555a)
|
@georgelesica-wf could we merge this. |
|
CI is broken right now and I haven't had time to fix it. It's not running our checks, just the tests. If someone can figure out how to get the code gen to run and can finish making this PR pass then we can start merging again. Until then I'd rather hold off. |
|
@boyan-soubachov Could you help reviewing this and merge. |
boyan-soubachov
left a comment
boyan-soubachov
left a comment
There was a problem hiding this comment.
Thank you for the contribution, just some questions.
Looking at the Run() function in suite.go, it looks quite unwieldy with a high cognitive complexity. Would refactoring make some sense there? Is there a quick, feasible refactor we can do there?
| @@ -0,0 +1,47 @@ | |||
| package suite | |||
There was a problem hiding this comment.
What is your logic for putting this in a separate file? Does it not make sense to just have this as another test in suite_test.go?
There was a problem hiding this comment.
The other file is huge now, thought could separate the tests based on what we test sortof integration vs small function unit tests.
There was a problem hiding this comment.
I see. I do agree that it's becoming a bit large but I don't think navigating it is still a huge problem.
I'd be happy to discuss any ideas you might have about improving its readability & maintainability, whether it involves splitting it up or some other way.
| test := testing.InternalTest{ | ||
| Name: method.Name, | ||
| F: func(t *testing.T) { | ||
| defer func() { |
There was a problem hiding this comment.
Can we not just defer testSync.Done() instead of wrapping it in an anonymous function?
There was a problem hiding this comment.
Had some logic which i've removed later. good catch, have updated.
boyan-soubachov
left a comment
boyan-soubachov
left a comment
There was a problem hiding this comment.
Looks great. Thank you for your contribution! 👍 😄
3 participants
Using WaitGroup to synchronise between tests so TearDown runs at end.
Having
suite.T().Parallel()inTestAcauses it to hang. goroutine wait in<-t.signalat testing.go and suite.go Run to wait ontestsSync:108Closes #466