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

Migrates e2e tests from ginkgo,gomega to testing,testify #130

Merged
merged 8 commits into from
Mar 31, 2021

Conversation

codefromthecrypt
Copy link
Contributor

@codefromthecrypt codefromthecrypt commented Mar 24, 2021

To call ginkgo,gomega indirect would be putting things lightly. A combination of an indirect testing library and long-running tests are a recipe for frustration. I've begun a process to remove ginkgo,gomega with a couple tests
that I had been looking at.

See #127

Some benefits

  • Before, we both had tests using normal testing,testify and also ginkgo,gomega. Now we only have way to write tests: testing,testify.
  • testing,testify are more popular and easier to learn than ginkgo,gomega. testing is the standard library described in all golang books. testify is a simple assertion library 10x more popular than gomega in terms of star count. Neither require learning BDD concepts.
  • Before, our running of e2e tests was significantly different than our other non-gingko tests in the same project. It literally required compilation of a platform-specific binary named 'e2e', adding a lot of complexity to CI and local setup.
  • Before, in order to run tests via normal means, you would have to create a suite wrapper file XXX_suite_test.go. This is no longer required, so we have less files to explain and maintain.
  • Before, individual tests cannot be executed via normal means, such as Goland. Instead, you have to run the suite file. Now, you can execute and more importantly debug individual tests instead of via a suite wrapper.
  • Before, writing tests not only required understanding of the subject, but also the ginkgo runtime. Otherwise, statements like var _ = Describe("getenvoy extension init", func() { seem pure magic.
  • Before, the nature of test execution required a combination of understanding the go platform and also ginkgo
  • Before, updating Ginkgo/gomega would skew the dependency tree, due to their dependencies on two protobuf libraries. Now, the test runtime does not interfere with main code.
  • Before, the same test took more lines of code to achieve the same table test. Less code is less to maintain.
  • Before, timeouts were hard to reason with: ginkgo defaults timeout to 24hrs and go test 10m. Using only one timeout system simplifies interpretation.
  • Some other notes about migration off are captured in this issue Use Go std testing library for unit tests openservicemesh/osm#1704

Notable differences

Gomega had a different way of logging. For example instead of using a logging library, it would use statements like this:
By("changing to the output directory") which translate into output STEP: changing to the output directory. This is mainly to satisfy BDD idioms and is meant to be interpreted amongst the many lines of normal log output. For cases where the points inside the test should be logged, we should use normal logging instead. However, I don't recommend making log statements unless they are meaningful and clarify progress.

Also, those not used to it will notice use of normal go TestMain and defer commands instead of the before/after test hooks used in tools like gomega. This can take getting used to, but it is also the same as normal go code, so shouldn't be time lost. There are some cases where multiple tear-down steps are needed and those steps are more visible now. That's not necessarily bad because these typically are also the most complex tests and have a lot going on. Seeing the various setup requirements shows problem areas that help knowing when debugging.

@codefromthecrypt
Copy link
Contributor Author

Here's an example of the tests executing in Goland

Screen Shot 2021-03-24 at 3 57 38 PM

@mathetake
Copy link
Member

+1 Personally, I'm all in for this migration, and I've been frustrated too.

I would like to have @yskopets's opinition.

@yskopets
Copy link
Member

@codefromthecrypt

A combination of an indirect testing library and long-running tests are a recipe for frustration.

Can you explain what it means ? What has changed after this PR ?

While the rationale is fairly straightforward, the following issue captures a lot of it nicely

Personally, I don't agree with this rationale.

I don't see from the linked issue how much support it received.

It's labeled with needs discussion for a reason.

If I understand the linked issue correctly, it has only 1 argument - "Let's move to Go std library because Ginkgo/Gomega is not a Go std library."

Sure. Developers choose Ginkgo/Gomega when they want to do BDD-style testing (which is uniform across all programming languages). If Go std library cannot offer it, developers have to switch to other libraries that can.

I've begun a process to remove gomega with a couple tests that I had been looking at.

I'm looking at the replacement code of this PR and I don't understand what component has become better now:

  • is it more readable ?
  • does it give better assertion messages when the test fails ?
  • does it leave a better trail in the test log ?

I don't mind finishing the e2e suite

It's a team/community work. If majority of contributors are in favour of this change, I won't object.

Please be mindful, though, about people who will have to maintain these tests.

If you're not sticking around to maintain the refactored code, please consider the effect of such changes on other developers.

@codefromthecrypt
Copy link
Contributor Author

The highest value from this so far is that the nested test is directly executable now, as opposed to unrunnable in Goland, for example. I would like to hear from others besides you @yskopets because I have noticed a lot of projects copy/paste things even when they don't agree with it. There are likely more people who have only tolerated this gomega thing due to difficulty getting rid of it.

@codefromthecrypt codefromthecrypt changed the title Documents getenvoy_extension_test_test.go and starts migration of gomega Documents getenvoy_extension_test_test.go and starts migration of ginkgo/gomega Mar 25, 2021
@codefromthecrypt
Copy link
Contributor Author

@mathetake I'd like to merge what's here. Later, I will do a PR to finish the rest, but landing incremental progress makes debugging #129 far easier, which is what started all of this really.

@codefromthecrypt
Copy link
Contributor Author

a different test failed while looking at go:embed and debuggability problems are the same as the one I've already converted. I'll go ahead and finish this PR vs doing things piece-wise. I don't usually like huge PRs, but I like whack-a-mole less

@codefromthecrypt
Copy link
Contributor Author

found this bug while visiting the push tests I'll rebase once it is merged #147

@codefromthecrypt
Copy link
Contributor Author

will be another day or two to finish. the "run" tests are rather gnarly so saved them for last :p

@codefromthecrypt
Copy link
Contributor Author

the "getenvoy extension run" unit tests are now done. I still need to do the e2e for that, and that's probably where I'll stop as the "getenvoy extension" tests were the more difficult ones I ran into, leading to this issue. I'll update soon and probably separate out the "unit tests" from the e2e.

I quote "unit tests" as they aren't really like normal unit tests, as they do invoke OS commands etc. The whole unit test run could be much faster if this was faked out, resulting in less temp dirs and process calls. I won't have time to address that, though.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt codefromthecrypt changed the title Documents getenvoy_extension_test_test.go and starts migration of ginkgo/gomega Migrates e2e tests from ginkgo,gomega to testing,testify Mar 30, 2021
@codefromthecrypt
Copy link
Contributor Author

I pulled the unit test changes into a separate PR as they were instrumental in figuring out what was going on in the e2e tests. #151

This test is purely addressing the e2e, but other benefits such as less dependencies won't be realized until other tests stop using gingko/gomega. Sadly, just this part took so much time I don't have time to port all remaining tests. OTOH, it should be far easier with this and #151 in.

@codefromthecrypt
Copy link
Contributor Author

run phases fail on timeout locally and in CI for rust. I'll look into it, but I raised an issue as generally speaking everything rust related takes a huge amount of time. We should probably bump the matrix so that the language e2e runs is managed externally and defaults to tinygo #152

Adrian Cole added 3 commits March 30, 2021 19:52
Signed-off-by: Adrian Cole <adrian@tetrate.io>
…ands

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Signed-off-by: Adrian Cole <adrian@tetrate.io>
…cripts

Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Contributor Author

kicked e2e tests due to #145 (current script we are using routinely times out installing docker on osx)

test/e2e/util/command.go Outdated Show resolved Hide resolved
Signed-off-by: Adrian Cole <adrian@tetrate.io>
@codefromthecrypt
Copy link
Contributor Author

codefromthecrypt commented Mar 31, 2021

thanks for the review and pointers @mathetake, as well the support behind the scenes. this is definitely better now

@codefromthecrypt
Copy link
Contributor Author

re-kicked the osx job as it is failing for the usual reason:

Waiting for docker service to be in the running state. 	If Docker is not ready within 10 minutes, it's better to fail the current CI Job and let the comitter to re-start the job manually

@mathetake mathetake requested a review from musaprg March 31, 2021 05:07
@mathetake
Copy link
Member

@musaprg any comments?

@musaprg
Copy link
Contributor

musaprg commented Mar 31, 2021

Personally, +1 to this PR, though I'm not sure it would be the best way in a community. I prefer the standard way in Go and I think it makes current tests more simple and readable.

@musaprg
Copy link
Contributor

musaprg commented Mar 31, 2021

also, ginkgo/gomega is a little bit complicated for contributors who new to them, for me at first as well.

Copy link
Member

@mathetake mathetake left a comment

Choose a reason for hiding this comment

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

Thanks @codefromthecrypt for this awesome work. Will merge after all the jobs pass

@mathetake
Copy link
Member

note that now we can run individual test in a click of button
Screenshot from 2021-03-31 14-00-31
Screenshot from 2021-03-31 14-00-18

@mathetake mathetake merged commit 7cd8354 into master Mar 31, 2021
@mathetake mathetake deleted the start-migration branch March 31, 2021 05:45
codefromthecrypt pushed a commit that referenced this pull request Mar 31, 2021
After #130, we can now easily run e2e tests in an IDE like Goland.
However, it is still tedious to have to specify `E2E_GETENVOY_BINARY`
every time. This removes this and also polishes the Makefile more.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt added a commit that referenced this pull request Mar 31, 2021
After #130, we can now easily run e2e tests in an IDE like Goland.
However, it is still tedious to have to specify `E2E_GETENVOY_BINARY`
every time. This removes this and also polishes the Makefile more.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt pushed a commit that referenced this pull request Apr 4, 2021
This completes migration of ./pkg/cmd/... from ginkgo,gomega to
testing,testify per rationale noted in #130.

This also fixes a bug where we accidentally added ignore files into
the output directory of `getenvoy extension init` and backfills tests
to ensure that doesn't happen again.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt pushed a commit that referenced this pull request Apr 6, 2021
This completes migration of ./pkg/cmd/... from ginkgo,gomega to
testing,testify per rationale noted in #130.

This also fixes a bug where we accidentally added ignore files into
the output directory of `getenvoy extension init` and backfills tests
to ensure that doesn't happen again.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt pushed a commit that referenced this pull request Apr 6, 2021
Before, we used an incorrect exit condition to determine if a process
still exists. Now, we look up the PID.

This converts test code to testing/testify per rationale in #130.
This upgrades testify to support error conditions.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt added a commit that referenced this pull request Apr 6, 2021
This completes migration of ./pkg/cmd/... from ginkgo,gomega to
testing,testify per rationale noted in #130.

This also fixes a bug where we accidentally added ignore files into
the output directory of `getenvoy extension init` and backfills tests
to ensure that doesn't happen again.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt pushed a commit that referenced this pull request Apr 6, 2021
Before, we used an incorrect exit condition to determine if a process
still exists. Now, we look up the PID.

This converts test code to testing/testify per rationale in #130.
This upgrades testify to support error conditions.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt pushed a commit that referenced this pull request Apr 6, 2021
Before, we used an incorrect exit condition to determine if a process
still exists. Now, we look up the PID.

This converts test code to testing/testify per rationale in #130.
This upgrades testify to support error conditions.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt added a commit that referenced this pull request Apr 6, 2021
Before, we used an incorrect exit condition to determine if a process
still exists. Now, we look up the PID.

This converts test code to testing/testify per rationale in #130.
This upgrades testify to support error conditions.

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt pushed a commit that referenced this pull request Apr 7, 2021
Per #130, this ports more tests to testing/testify, leaving only
pkg/extension/workspace left (for another pull request).

Signed-off-by: Adrian Cole <adrian@tetrate.io>
codefromthecrypt added a commit that referenced this pull request Apr 8, 2021
Per #130, this ports more tests to testing/testify, leaving only
pkg/extension/workspace left (for another pull request).

Signed-off-by: Adrian Cole <adrian@tetrate.io>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants