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

PF4: (virtualized-extension) jest tests for VirtualizedTable #2761

Merged
merged 15 commits into from Oct 25, 2019

Conversation

@jenny-s51
Copy link
Contributor

jenny-s51 commented Aug 22, 2019

WIP for #2328 -- currently working on resolving these type errors shown after running yarn run jest virtualizedTable.

Screen Shot 2019-08-22 at 3 42 47 PM

@jenny-s51 jenny-s51 requested a review from priley86 Aug 22, 2019
@jenny-s51

This comment has been minimized.

Copy link
Contributor Author

jenny-s51 commented Aug 23, 2019

Resolving TS errors with any type for now. Can change the types to what's more appropriate, but need some feedback on what would be best here. In addition, /Virtualized and/Virtualized/utils are now showing up in the coverage report, but coverage still really needs to be improved (13.23% and 19.29%, respectively). Will be working on this.

@@ -6,3 +6,24 @@ declare module 'victory-core' {
export const Path: any;
export const TextSize: any;
}

declare module 'linear-layout-vector' {

This comment has been minimized.

Copy link
@priley86

priley86 Aug 23, 2019

Member

hey @jenny-s51 - my only ounce of feedback so far here would be - can we move this declaration into patternfly-4/react-virtualized-extension some place? I believe linear-layout-vector is only a dependency of that package (so probably unnecessary in react-core).

Also, i think you can safely remove package-lock.json as well. We don't ship lock files to consumers (just reference the dependences in the package.json for each package) and all of the dev dependencies for PF React end up getting locked in the root yarn.lock file (which should update when you install new packages or run yarn install in the root).

Great job w/ this, it's a huge help!!! ⭐️

@tlabaj tlabaj assigned dlabaj and mturley and unassigned mturley Aug 29, 2019
@tlabaj tlabaj requested a review from mturley Aug 29, 2019
@jenny-s51 jenny-s51 force-pushed the jenny-s51:iss2328 branch from ed86deb to c79f917 Sep 9, 2019
@priley86

This comment has been minimized.

Copy link
Member

priley86 commented Sep 9, 2019

hey @jenny-s51 - i've pushed a commit w/ some proposed changes to this PR today:
https://github.com/priley86/patternfly-react/commits/iss2328

Let me know what you think. I'm mostly happy here aside from one warning in the jest log still...

 PASS  packages/patternfly-4/react-virtualized-extension/src/components/Virtualized/VirtualizedTable.test.tsx
  ● Console

    console.error node_modules/prop-types/checkPropTypes.js:20
      Warning: Failed prop type: Invalid prop `children` supplied to `Table`, expected a ReactNode.
          in Table
    console.error packages/patternfly-4/react-table/dist/js/components/Table/Table.js:237
      Table: Specify at least one of: header, caption, aria-label
    console.error node_modules/prop-types/checkPropTypes.js:20
      Warning: Failed prop type: Invalid prop `children` supplied to `Provider`, expected a ReactNode.
          in Provider (created by Table)
          in Table (created by WrapperComponent)
          in WrapperComponent
    console.error node_modules/react-dom/cjs/react-dom.development.js:180
      Warning: Functions are not valid as a React child. This may happen if you return a Component instead of <Componen
t /> from render. Or maybe you meant to call this function rather than return it.
          in table (created by Provider)
          in Provider (created by Table)
          in Table (created by WrapperComponent)
          in WrapperComponent
    console.error packages/patternfly-4/react-table/dist/js/components/Table/Table.js:237
      Table: Specify at least one of: header, caption, aria-label

Did not get a chance to address it. I will try testing these changes downstream tomorrow and report back though!

@jenny-s51 jenny-s51 force-pushed the jenny-s51:iss2328 branch from 824aee7 to 2e10b10 Sep 16, 2019
@patternfly-build

This comment has been minimized.

Copy link
Contributor

patternfly-build commented Sep 16, 2019

PatternFly-React preview: https://patternfly-react-pr-2761.surge.sh

@jenny-s51 jenny-s51 force-pushed the jenny-s51:iss2328 branch from 3790ffd to 2e10b10 Sep 16, 2019
@@ -180,7 +180,7 @@ export interface TableProps {
contentId?: string;
dropdownPosition?: 'right' | 'left';
dropdownDirection?: 'up' | 'down';
rows: (IRow | string[])[];
rows?: (IRow | string[])[];

This comment has been minimized.

Copy link
@priley86
@@ -117,5 +117,8 @@
"packages": [
"packages/**"
]
},
"dependencies": {
"@types/react-virtualized": "^9.21.4"

This comment has been minimized.

Copy link
@priley86

priley86 Sep 25, 2019

Member

i don't think this is needed in the root anymore since we've added it in react-virtualized-extension's package.json

This comment has been minimized.

Copy link
@jenny-s51

jenny-s51 Sep 26, 2019

Author Contributor

how would you suggest I address this? would it just be yarn remove @types/react-virtualized?

This comment has been minimized.

Copy link
@mturley

mturley Oct 21, 2019

Collaborator

I think so, yes, or you could remove the line from the package.json and re-run yarn install. If the build still passes without this dependency in here, yeah, I think remove it. But it's probably ok either way.

This comment has been minimized.

Copy link
@redallen

redallen Oct 22, 2019

Contributor

You can delete the line. Likely no need to re-run yarn install, but better safe than sorry.

@@ -179,6 +179,16 @@ test('Collapsible nested table', () => {
expect(view).toMatchSnapshot();
});

test('Header only table', () => {

This comment has been minimized.

Copy link
@priley86
@@ -1,6 +1,6 @@
/* eslint-disable */

import LinearLayoutVector from 'linear-layout-vector';
const LinearLayoutVector = require("linear-layout-vector");

This comment has been minimized.

Copy link
@priley86

priley86 Sep 25, 2019

Member

is this still necessary? (changing import to require)

@priley86

This comment has been minimized.

Copy link
Member

priley86 commented Sep 25, 2019

hey @jenny-s51 - i'm mostly OK w/ this change. I would just remove any package-lock.json files (since we havent been checking those in)

@priley86 priley86 requested review from redallen and seanforyou23 Sep 25, 2019
@mturley

This comment has been minimized.

Copy link
Collaborator

mturley commented Sep 25, 2019

@jenny-s51 +1 about package-lock.json.. and actually, if that file is being created you're probably using the npm CLI, I'd make sure you're running yarn install instead of npm install because we do want any updates to yarn.lock 🙂

@jenny-s51

This comment has been minimized.

Copy link
Contributor Author

jenny-s51 commented Sep 26, 2019

@priley86 do you mean I can just remove package-lock.json from this PR entirely?

@redallen

This comment has been minimized.

Copy link
Contributor

redallen commented Sep 26, 2019

@jenny-s51 We use yarn with yarn.lock to install packages, so yes please remove package-lock.json!

@jenny-s51

This comment has been minimized.

Copy link
Contributor Author

jenny-s51 commented Sep 26, 2019

@redallen gotcha, should be all set now -- thank you!

@jenny-s51 jenny-s51 force-pushed the jenny-s51:iss2328 branch from 577a6e2 to c539616 Sep 27, 2019
@jenny-s51 jenny-s51 requested a review from priley86 Sep 27, 2019
@jenny-s51 jenny-s51 force-pushed the jenny-s51:iss2328 branch from b929d46 to 29ebfa6 Oct 15, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 15, 2019

Codecov Report

Merging #2761 into master will decrease coverage by 2%.
The diff coverage is 36.36%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2761      +/-   ##
==========================================
- Coverage   69.13%   67.12%   -2.01%     
==========================================
  Files         868      873       +5     
  Lines       23777    24490     +713     
  Branches     1932     2071     +139     
==========================================
+ Hits        16438    16439       +1     
- Misses       6360     7088     +728     
+ Partials      979      963      -16
Flag Coverage Δ
#misc 95.45% <ø> (ø) ⬆️
#patternfly3 69.3% <ø> (ø) ⬆️
#patternfly4 63.99% <36.36%> (-4.3%) ⬇️
Impacted Files Coverage Δ
...ts/Virtualized/utils/CellSizeAndPositionManager.ts 16.49% <ø> (ø)
...src/components/Virtualized/utils/animationFrame.ts 27.27% <ø> (ø)
...onents/Virtualized/utils/createCallbackMemoizer.ts 0% <0%> (ø)
...components/Virtualized/defaultCellRangeRenderer.ts 1.81% <0%> (ø)
...on/src/components/Virtualized/VirtualTableBody.tsx 18.96% <0%> (ø)
...ualized/utils/ScalingCellSizeAndPositionManager.ts 27.94% <100%> (ø)
...nents/Virtualized/utils/requestAnimationTimeout.ts 27.27% <100%> (ø)
...xtension/src/components/Virtualized/VirtualGrid.ts 9.3% <33.33%> (ø)
...-4/react-core/src/components/TextArea/TextArea.tsx 93.75% <0%> (-1.71%) ⬇️
...ly-4/react-core/src/components/Tooltip/Tooltip.tsx 84.48% <0%> (-0.27%) ⬇️
... and 30 more

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 672047a...48756dd. Read the comment docs.

@jenny-s51 jenny-s51 force-pushed the jenny-s51:iss2328 branch 3 times, most recently from 36a8485 to 5713a58 Oct 15, 2019
@jenny-s51 jenny-s51 force-pushed the jenny-s51:iss2328 branch 4 times, most recently from be32d96 to 6921275 Oct 24, 2019
@jenny-s51 jenny-s51 force-pushed the jenny-s51:iss2328 branch from 38301b6 to 3de0bfb Oct 25, 2019
@jenny-s51 jenny-s51 requested a review from mturley Oct 25, 2019
@mturley mturley merged commit bb31325 into patternfly:master Oct 25, 2019
8 checks passed
8 checks passed
ci/circleci: build Your tests passed on CircleCI!
Details
ci/circleci: build_integration Your tests passed on CircleCI!
Details
ci/circleci: build_pf3_docs Your tests passed on CircleCI!
Details
ci/circleci: build_pf4_docs Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: test_jest_other Your tests passed on CircleCI!
Details
ci/circleci: test_jest_pf4 Your tests passed on CircleCI!
Details
ci/circleci: upload_docs Your tests passed on CircleCI!
Details
@jenny-s51 jenny-s51 deleted the jenny-s51:iss2328 branch Oct 25, 2019
redallen added a commit to redallen/patternfly-react that referenced this pull request Oct 28, 2019
kmcfaul added a commit that referenced this pull request Oct 29, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.