Test DependencyManagementPane component #363

AWolf81 · 2019-02-10T20:46:40Z

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:

Moved components to DependencyManagementPane folder (see discussion below for more details)
Updated Enzyme & enzyme-adapter-react-16 for renderProps support
The --verbose=false option in the test script enables console.log during test (I'd like to add it as it's quite handy to log some stuff - no need to start a debug seesion in VS code)
Tried to limit the size of each snapshot to see what eslint rule would be good. I think 100 lines would be a good value. See AddDependencySearchResult.test.js how to limit a snapshot. I'll setup linting in a separate PR.

Discussion

Should we move more related components into DependencyManagementPane folder? I think DeleteDependencyButton, DependencyDetailsTable & DependencyInfoFromNpm would also fit into that folder but I'm not sure. Is there a way to see where components are used so I could see if theses where also used at other locations? I moved the components to DependencyManagementPane folder to have a better grouping and I think these components are only used there - so it's OK to do the grouping.
Moving tests into a __tests__ folder? Same question as in CreateNewProjectWizard PR. Should we move all tests into a subdirectory if there are many tests in a folder e.g. more than 2 tests? (Would probably reduce some noise)

codecov · 2019-02-10T21:21:34Z

Codecov Report

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

@@            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

AWolf81 · 2019-03-22T11:46:17Z

package.json

@@ -28,7 +28,7 @@
    "dist:all": "cross-env GENERATE_SOURCEMAP=false npm run build && electron-builder -mwl -c.extraMetadata.main=build/electron.js",
    "publish": "yarn dist --publish always",
    "publish:all": "yarn dist:all --publish always",
-    "test": "node scripts/test.js --maxWorkers=2 --env=node",
+    "test": "node scripts/test.js --maxWorkers=2 --env=node --verbose=false",


Note: I'll remove verbose later today. So we're getting the default output. With verbose=false console.logs are visible in the console but not every test step.

Haroenv · 2019-03-22T11:58:05Z

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

melanieseltzer

Should we move more related components into DependencyManagementPane folder? I think DeleteDependencyButton, DependencyDetailsTable & DependencyInfoFromNpm would also fit

Agreed - they should be moved under DependencyManagementPane. I did a search across the codebase for DeleteDependencyButton, DependencyDetailsTable, DependencyInfoFromNpm, and also DependencyUpdateRow to see where they're imported and they don't seem to be used anywhere other than DependencyDetails (which is already under DependencyManagementPane). And let's go with the __tests__ folder pattern here too since these are many files.

I think this is good to go after that! Thanks @Haroenv for also taking a look 👍

superhawk610 · 2019-03-26T22:03:08Z

@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 · 2019-03-26T22:31:15Z

@superhawk610 It looks like --verbose=false is the recommended method to enable console logging :/ Seems like they get swallowed somehow which is strange. Per jestjs/jest#2441 (comment), 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 · 2019-03-26T22:40:09Z

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 · 2019-03-26T23:02:01Z

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 · 2019-03-26T23:22:33Z

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 · 2019-03-26T23:30:20Z

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",

* moved related components to DependencyManagementPane * added tests * fixed flow * added a constant instead of Date.now() * updated snapshot * removed white-spaces * removed verbose * fixed linting & mocked AlgoliaLogo * move related component files & moved tests to __tests__ folder * Add script for test:dev

* add analytics tags * Update AddDependencySearchProvider.js * #352 Disable actions that require an internet connection (#368) * Online check component added * add reducer & action for online check component. also included in App.js * removed sidebar functionality when offline * removed depedency functionality when offline * removed functionality from create new project wizard when offline * fixed tests to align with disabled pattern * add infoBanner to Z-indexes for future usage. * removed random {' '}, changed IS_ONLINE_CHECK to SET_ONLINE_STATUS * pull request requested changes missed one file. * updated infobar to be fixed header. * flow fix * remove random whitespace * Pull request changes remove state type, move styled components to before redux part, change background color of infobar to be transparent. * enable menu items based on isOnline * Setup ESlint rules for Jest (#366) * WIP: Added eslint-jest & fixed issues (except snapshots) * reduced snapshot size to meet max 100 lines * fixed snapshot * changed comment to a note about noPadding prop * updated snapshot * Test ProjectConfigurationModal component (#360) * WIP: Added render test * WIP: Added tests * fixed flow * added focus test * Merge branch 'master' into test-project-configuration-modal * fixed linting warnings * removed typing as no flow used in test files * Test CreateNewProjectWizard components (#361) * WIP: Added tests * WIP: ProjectType undefined not throwing in test * fixed flow * added tests * reverted to children render props * Merge branch 'master' into test-create-new-project-wiz * Fixed linting & replaced snapshot with simple smoke tests * addressed review comments * Test DependencyManagementPane component (#363) * moved related components to DependencyManagementPane * added tests * fixed flow * added a constant instead of Date.now() * updated snapshot * removed white-spaces * removed verbose * fixed linting & mocked AlgoliaLogo * move related component files & moved tests to __tests__ folder * Add script for test:dev * Fix spaces and update lint-staged (#369) * Switch to advanced config to silence validation warning * Fix spacing issues * chore: fix merge changes

AWolf81 requested review from idoberko2, melanieseltzer and superhawk610 February 10, 2019 20:46

AWolf81 added 6 commits March 22, 2019 00:30

moved related components to DependencyManagementPane

cacc9ae

added tests

81f522b

fixed flow

a43779e

added a constant instead of Date.now()

8c61992

updated snapshot

5c6fac7

removed white-spaces

eda3093

AWolf81 force-pushed the test-dependency-management-pane branch from 53b2374 to eda3093 Compare March 21, 2019 23:52

AWolf81 commented Mar 22, 2019

View reviewed changes

AWolf81 added 3 commits March 24, 2019 00:45

Merge branch 'master' into test-dependency-management-pane

3615fc9

removed verbose

d696d98

fixed linting & mocked AlgoliaLogo

c861140

melanieseltzer reviewed Mar 26, 2019

View reviewed changes

move related component files & moved tests to __tests__ folder

df24854

Add script for test:dev

d5552ec

melanieseltzer approved these changes Mar 26, 2019

View reviewed changes

melanieseltzer merged commit 6eaab78 into master Mar 26, 2019

melanieseltzer deleted the test-dependency-management-pane branch April 4, 2019 06:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Test DependencyManagementPane component #363

Test DependencyManagementPane component #363

AWolf81 commented Feb 10, 2019 •

edited

Loading

codecov bot commented Feb 10, 2019 •

edited

Loading

AWolf81 Mar 22, 2019

Haroenv commented Mar 22, 2019

melanieseltzer left a comment

superhawk610 commented Mar 26, 2019

melanieseltzer commented Mar 26, 2019

superhawk610 commented Mar 26, 2019

melanieseltzer commented Mar 26, 2019

superhawk610 commented Mar 26, 2019

AWolf81 commented Mar 26, 2019

Test DependencyManagementPane component #363

Test DependencyManagementPane component #363

Conversation

AWolf81 commented Feb 10, 2019 • edited Loading

codecov bot commented Feb 10, 2019 • edited Loading

Codecov Report

AWolf81 Mar 22, 2019

Choose a reason for hiding this comment

Haroenv commented Mar 22, 2019

melanieseltzer left a comment

Choose a reason for hiding this comment

superhawk610 commented Mar 26, 2019

melanieseltzer commented Mar 26, 2019

superhawk610 commented Mar 26, 2019

melanieseltzer commented Mar 26, 2019

superhawk610 commented Mar 26, 2019

AWolf81 commented Mar 26, 2019

AWolf81 commented Feb 10, 2019 •

edited

Loading

codecov bot commented Feb 10, 2019 •

edited

Loading