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

Automated integration tests for parent-level commands #70

Merged
merged 6 commits into from
Jun 17, 2020
Merged

Automated integration tests for parent-level commands #70

merged 6 commits into from
Jun 17, 2020

Conversation

brandongregoryscott
Copy link
Contributor

@brandongregoryscott brandongregoryscott commented Jun 9, 2020

Issue #46

First pass at setting up integration tests for the parent-level commands to help prevent regressions at the level that connects module code together.

Summary of changes:

  • Added test-utils file to hold test-specific functionality (spawning processes, creating tmp directories, verifying environment setup)
  • Updated the way shelljs and child_process are mocked (actual jest mocks instead of function replacements, so the actual implementation can be restored if needed)
  • Added a test file for each parent command. Most of these test files just run a test to make sure that -h and --help flags display the help menu for the command.
    • The cli.test.js file also tests that every command in _modules/commands.js is displayed in the help menu
    • The cli-dotnet.test.js file tests that running -b or --build without a dotnet solution below the current directory prints an error.
    • The cli-dotnet.test.js file tests that a fresh solution with a console app project successfully builds.
  • Moved setupTests.js to tests/ subfolder, as well as adding a specific file for "setupAfterEnv" config setting for Jest.
  • In order to have the dotnet sdk on Travis, I updated the language and added a dotnet version in the build script, based on our current C# repos with automated builds. Travis gives you nvm out of the box, so we can still target a specific node version and install dependencies by leveraging that.

@wintondeshong This is a pretty hefty update to the project, so I would love to get some feedback from you on the infrastructure I setup for the test suite. No rush on this, but I agree with the increased need for test coverage at this level.

@brandongregoryscott brandongregoryscott requested review from wintondeshong and a team June 9, 2020 13:26
@codecov
Copy link

codecov bot commented Jun 9, 2020

Codecov Report

Merging #70 into master will increase coverage by 4.18%.
The diff coverage is 94.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #70      +/-   ##
==========================================
+ Coverage   69.34%   73.52%   +4.18%     
==========================================
  Files          13       16       +3     
  Lines         398      476      +78     
  Branches       66       74       +8     
==========================================
+ Hits          276      350      +74     
- Misses        101      105       +4     
  Partials       21       21              
Impacted Files Coverage Δ
_modules/echo.js 40.74% <87.50%> (+4.74%) ⬆️
tests/test-utils.js 90.90% <90.90%> (ø)
_modules/constants.js 100.00% <100.00%> (ø)
_modules/dotnet-build.js 95.83% <100.00%> (+0.18%) ⬆️
_modules/dotnet-path.js 100.00% <100.00%> (ø)
tests/describes.js 100.00% <100.00%> (ø)
_modules/commands.js 100.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1745165...cbab20c. Read the comment docs.

@brandongregoryscott brandongregoryscott changed the title Issue #46 Automated integration tests for parent-level commands Automated integration tests for parent-level commands Jun 9, 2020
Copy link
Contributor

@wintondeshong wintondeshong left a comment

Choose a reason for hiding this comment

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

Gave the suite a run and worked as expected 👍 Nice work!!!

While there is nothing functionally wrong, I do feel that the proposed shared test/spec for the help tests is significant enough of an impact on the code base, that it should be done before I accept. Heck, could you write a test that also just loops over 'commands.js' and runs help on them? This might automatically ensure new entries get that test? Food for thought on that one.

@@ -1,18 +1,22 @@
version: "~> 1.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, so to test the dotnet related commands, you are installing node on a dotnet/csharp node. This works for now, but does bring up the point that we could very well get into a scenario where we need to have multiple travis build environments as the CLI works with even other project types. No need to go down that road now of course.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For sure. I wasn't sure if it would be better to manually install the dotnet dependencies in the script itself, or use the community dotnet template. I chose the path of less resistance for now 😛

cli-copy.test.js Outdated Show resolved Hide resolved
cli-deploy-aws-beanstalk.test.js Outdated Show resolved Hide resolved
cli-copy.test.js Outdated Show resolved Hide resolved
cli-dotnet.test.js Outdated Show resolved Hide resolved
tests/test-utils.js Outdated Show resolved Hide resolved
@brandongregoryscott
Copy link
Contributor Author

Gave the suite a run and worked as expected 👍 Nice work!!!

While there is nothing functionally wrong, I do feel that the proposed shared test/spec for the help tests is significant enough of an impact on the code base, that it should be done before I accept. Heck, could you write a test that also just loops over 'commands.js' and runs help on them? This might automatically ensure new entries get that test? Food for thought on that one.

I think that might be the way to go - it would definitely save our butts in the event that someone forgets to add the help test boilerplate (whether or not we abstract that out into a shared test/spec.) The test files for each command could then be more specific to the functionality of the command itself.

@brandongregoryscott
Copy link
Contributor Author

@wintondeshong I believe I have captured all of your feedback. The biggest refactor was the tests/describes.js file - feel free to take a look at that and any of the cli-*.test.js files to see how it is used.

While I kept those help tests in there, I also added a looped test in the cli.js test file to ensure that any commands registered through regular means (_modules/commands.js) get that test for free.

Copy link
Contributor

@wintondeshong wintondeshong left a comment

Choose a reason for hiding this comment

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

@brandongregoryscott totally dig the shared test helper. good to go after renaming of the _whenGivenOptions shared test and removing the word when in a few of the tests that state when given...

tests/describes.js Outdated Show resolved Hide resolved
cli.test.js Outdated Show resolved Hide resolved
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.

None yet

3 participants