Skip to content

Conversation

stuartpa
Copy link
Collaborator

@stuartpa stuartpa commented Sep 20, 2022

This is the 2nd part for the modern-CLI. See PR165 for first part:

This PR moves the file-layout for the modern CLI to conform to the idiomatic golang layout, and focuses on test and code coverage, there is no new functionality in this PR. The Unit test coverage for the modern sqlcmd.exe now 95%+.

The formatting of the MIT license file was part of @shueybubbles original commit, I presume we want to keep these.

TODO:

  • A follow up PR will at the ./cmd/modern folder (that builds the new sqlcmd.exe) to the build pipelines.
  • Right now, three tests are skipped, TestQuery, TestNegUninstallNoInstanceToUninstall and TestUnicodeInput, I am debugging these now

@stuartpa stuartpa changed the title barebones Cobra layout for new CLI Cobra layout for modern look Sep 20, 2022
@shueybubbles
Copy link
Collaborator

shueybubbles commented Oct 14, 2022

we should commit .editorconfig so everyone gets the same behavior. the go tooling handles most of it ok but if there are options the go tools don't enforce automatically we should use this file for consistency.


In reply to: 1279168195


In reply to: 1279168195


In reply to: 1279168195


In reply to: 1279168195


Refers to: .gitignore:27 in 3db8294. [](commit_id = 3db8294, deletion_comment = False)

@stuartpa
Copy link
Collaborator Author

Done


In reply to: 1279168195


Refers to: .gitignore:27 in 3db8294. [](commit_id = 3db8294, deletion_comment = False)

@stuartpa stuartpa force-pushed the shueybubbles/bcp branch 2 times, most recently from 2f840eb to ca118d2 Compare December 9, 2022 08:44
@stuartpa stuartpa marked this pull request as ready for review December 12, 2022 16:35
@stuartpa stuartpa changed the title Cobra layout for modern look Modern POSIX style CLI for SQLCMD - Part 2 Dec 12, 2022

func TestMainStart(t *testing.T) {
os.Args[1] = "--help"
main()
Copy link
Collaborator

Choose a reason for hiding this comment

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

main()

"doesn't panic" seems like a very minimal test. Couldn't this test verify that rootCmd is constructe with proper values?

@shueybubbles
Copy link
Collaborator

the structure looks ok to me. I don't see much value in unit tests that exercise a function with no assertions about the behavior. A unit test should serve as a specification (code > English) for a method, but without assertions there's no definition of correct behavior other than "it doesn't panic". With the dependency structure you have established, unit tests should have an easy ability to inject an interface implementation into the dependencies and verify that the method under test utilized that dependency correctly, much the way C# tests do with Moq. That way the unit test establishes the relationship between a given method and its dependencies and can truly prevent regressions in the future.

Copy link
Collaborator

@shueybubbles shueybubbles left a comment

Choose a reason for hiding this comment

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

:shipit:

@stuartpa stuartpa merged commit 39f8c1d into main Dec 27, 2022
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