joshwcomeau / guppy

🐠A friendly application manager and task runner for React.js
ISC License
3.27k stars 154 forks source link

Test DependencyManagementPane component #363

Closed AWolf81 closed 5 years ago

AWolf81 commented 5 years ago

This is quite a large PR so I'm not going into every test detail. I've tried to hit a coverage of over 80% and to reduce the size per snapshot.

Related Issue:

309

Summary:

Discussion

codecov[bot] commented 5 years ago

Codecov Report

Merging #363 into master will increase coverage by 1.61%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #363      +/-   ##
==========================================
+ Coverage   37.54%   39.15%   +1.61%     
==========================================
  Files         158      158              
  Lines        3540     3540              
  Branches      449      449              
==========================================
+ Hits         1329     1386      +57     
+ Misses       1928     1882      -46     
+ Partials      283      272      -11
Impacted Files Coverage Δ
...nts/DependencyManagementPane/AddDependencyModal.js 100% <ø> (ø)
...DependencyManagementPane/DeleteDependencyButton.js 20% <ø> (ø)
...DependencyManagementPane/DependencyDetailsTable.js 38.46% <ø> (ø)
...components/DependencyManagementPane/AlgoliaLogo.js 50% <ø> (ø)
...dencyManagementPane/AddDependencySearchProvider.js 100% <ø> (ø)
...DependencyManagementPane/AddDependencySearchBox.js 100% <ø> (ø)
.../DependencyManagementPane/DependencyInfoFromNpm.js 18.75% <ø> (ø)
...ts/DependencyManagementPane/DependencyUpdateRow.js 36.84% <ø> (ø)
...ents/DependencyManagementPane/DependencyDetails.js 100% <ø> (ø)
...s/DependencyManagementPane/DependencyInstalling.js 100% <ø> (ø)
... and 11 more
Haroenv commented 5 years ago

I didn't have a deep look, but at least on surface these changes look good :)

superhawk610 commented 5 years ago

@melanieseltzer on #367:

Specifying [--verbose=false] kills the reporting for each individual test if run via filename regex pattern.

Is there another flag to enable logging during test runs without disabling verbose output for single tests?

Corollary, isn't console logging enabled by default during test runs?

melanieseltzer commented 5 years ago

@superhawk610 It looks like --verbose=false is the recommended method to enable console logging :/ Seems like they get swallowed somehow which is strange. Per https://github.com/facebook/jest/issues/2441#issuecomment-458862213, seems people are getting inconsistent logging.

Ultimately if the logging is important let's just add the verbose=false in, I don't mind removing it when necessary since the verbose output is only useful to me when I'm reviewing larger PRs file-by-file (to get a quick overview of each test case). Or maybe two separate package.json scripts? :)

superhawk610 commented 5 years ago

Ahh, gotcha. Yeah I'm all about adding more package.json scripts. What's a logical place to leave an explanation for why there's two test scripts? Standard JSON sadly doesn't support any real sort of comments :(

melanieseltzer commented 5 years ago

There's this solution for commenting however personally that just seems clunky to me. I think this would be pretty self explanatory?

"test": "node scripts/test.js --env=node --verbose=false",
"test-verbose": "node scripts/test.js --env=node",
superhawk610 commented 5 years ago

You're right, that's pretty immediately understandable. I'd probably go with test and test:verbose (with a colon instead of a dash) for consistency with the other scripts.

AWolf81 commented 5 years ago

I think it would be better to have the default with-out verbose=false as this is what we'd like to see most of the time. So I'd prefer somthing like:

"test": "node scripts/test.js --maxWorkers=2 --env=node",
"test:dev": "yarn test --verbose=false",