Skip to content

bin: add a flag to list the subsystems #67

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

Merged
merged 5 commits into from
Jul 15, 2019

Conversation

lholmquist
Copy link
Contributor

This commit adds the flag --list-subsystems to list all the subsystems that are available for use.

fixes #66

Since the output is rather long is just listing in one column, so i'm using 3 columns when doing the listing.

This commit adds the flag --list-subsystems to list all the subsystems that are available for use.

fixes nodejs#66
@lholmquist
Copy link
Contributor Author

of course i didn't run npm test before i created this 😢 let me fix those linter issue quick

@codecov
Copy link

codecov bot commented Jul 11, 2019

Codecov Report

Merging #67 into master will decrease coverage by 40.93%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master      #67       +/-   ##
===========================================
- Coverage   98.07%   57.14%   -40.94%     
===========================================
  Files          13       18        +5     
  Lines         208      448      +240     
===========================================
+ Hits          204      256       +52     
- Misses          4      192      +188
Impacted Files Coverage Δ
bin/cmd.js 29.21% <100%> (ø)
lib/utils.js 58.06% <100%> (ø)
lib/tap.js 5.26% <0%> (ø)
lib/format-pretty.js 9.52% <0%> (ø)
lib/format-tap.js 4.76% <0%> (ø)

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 5740678...ffa5aae. Read the comment docs.

@richardlau
Copy link
Member

I also wonder if it makes sense to sort the printed list?

Trott
Trott previously approved these changes Jul 11, 2019
@Trott Trott dismissed their stale review July 11, 2019 17:50

whoops

@Trott
Copy link
Member

Trott commented Jul 11, 2019

One other thing: Would you be willing to add a test for this functionality?

@lholmquist
Copy link
Contributor Author

yes to the test

@lholmquist
Copy link
Contributor Author

lholmquist commented Jul 11, 2019

What did you have in mind as a test for this. It doesn't actually return anything, just prints to the console.

I guess maybe we could test that the appropriate flag was called?

@Trott
Copy link
Member

Trott commented Jul 11, 2019

What did you have in mind as a test for this. It doesn't actually return anything, just prints to the console.

I guess maybe we could test that the appropriate flag was called?

Good question. Maybe create a new file in test called test/cli.js or something like that, and have it use child_process to launch bin/cmd.js with the appropriate flag, and make sure that stdout and stderr have the expected results?

@lholmquist
Copy link
Contributor Author

That sounds good. that will also be a good starting point for adding tests for the other flags in the future

@lholmquist
Copy link
Contributor Author

I've added a test for the --list-subsytems flag.

I went back and forth on how this test should actually be. I ended up just doing a string compare of what was in my terminal and what outputs from the child process stdout.

I also thought about extracting the outputted subsystems into an array and then comparing that against the list of subsystems available to make sure they were all there. maybe that is thinking to much about it :)

@richardlau
Copy link
Member

I've added a test for the --list-subsytems flag.

I went back and forth on how this test should actually be. I ended up just doing a string compare of what was in my terminal and what outputs from the child process stdout.

I also thought about extracting the outputted subsystems into an array and then comparing that against the list of subsystems available to make sure they were all there. maybe that is thinking to much about it :)

I'm okay with the test as-is, but if you want to go the extra mile then that would mean the test wouldn't have to be manually updated in the future if more subsystems are added. It could also check, for example, that no subsystem appears more than once in the list.

@lholmquist
Copy link
Contributor Author

I'm going to refactor and do the list comparison.

@lholmquist
Copy link
Contributor Author

I've refactored the test to now compare the list of subsystems that were output with the original list. Doing it this way I think makes a little more sense that trying to compare the visual representation of it

@richardlau richardlau merged commit 30145f9 into nodejs:master Jul 15, 2019
@lholmquist lholmquist deleted the 66-list-available-subsystems branch July 15, 2019 17:55
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.

List available subsystems
3 participants