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

Rewrites trickiest cmd tests to help clarify what's happening #151

Merged
merged 4 commits into from
Apr 1, 2021

Conversation

codefromthecrypt
Copy link
Contributor

@codefromthecrypt codefromthecrypt commented Mar 30, 2021

Before, the cmd unit tests were difficult to understand. They are still
complicated and difficult, but far better documented now. Moreover, by
using standard testing libs, individual tests and even table cells can
be driven by the IDE, and failures point to a relevant location in a
table also.

See #130 for further rational about ginkgo/gomega vs testing/testify

@codefromthecrypt
Copy link
Contributor Author

opened #154 about the flakey test as there are about 4 that could break the same way

@mathetake
Copy link
Member

Finally I've looked at every case and understood them. LGTM so far (I want to have cmd_test.go free of Ginkgo as in my comment).

@codefromthecrypt
Copy link
Contributor Author

codefromthecrypt commented Apr 1, 2021

thanks for the hours spent on an epic review @mathetake

It took me several days to understand the tests to write this, so I'm glad two of us understand now. Most importantly, once this is in, we can fix some really concerning root problems that make tests so difficult, such as reliance on actually invoking executables vs stubbing. For now, I'm glad we can progress to make future progress less trouble. Will address your commits now.

Adrian Cole added 4 commits April 1, 2021 15:02
Before, the cmd unit tests were difficult to understand. They are still
complicated and difficult, but far better documented now. Moreover, by
using standard testing libs, individual tests and even table cells can
be driven by the IDE, and failures point to a relevant location in a
table also.

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

actually there are still a lot of tests left in the package. To save you from a blinding amount of unrelated changes, I kept the code related to your direct comments and commit to migrating the other tests in a subsequent PR, sg @mathetake?

@mathetake
Copy link
Member

sg

@codefromthecrypt
Copy link
Contributor Author

re-kicked as the osx thing is busted

@codefromthecrypt codefromthecrypt merged commit d1ce5a3 into master Apr 1, 2021
@codefromthecrypt
Copy link
Contributor Author

appreciate your at least 4 hours on this @mathetake!

@codefromthecrypt codefromthecrypt deleted the cmd-tests branch April 1, 2021 08:56
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.

2 participants