-
Notifications
You must be signed in to change notification settings - Fork 23
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
RHCLOUD-27198: Improve Cypress testing of sample plugin host application #232
Conversation
caac42c
to
208865c
Compare
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #232 +/- ##
=======================================
Coverage 41.16% 41.16%
=======================================
Files 69 69
Lines 1693 1693
Branches 365 365
=======================================
Hits 697 697
Misses 956 956
Partials 40 40 ☔ View full report in Codecov by Sentry. |
@vojtechszocs: This pull request references RHCLOUD-27198 which is a valid jira issue. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
packages/lib-core/src/index.ts
Outdated
|
||
// Testing utilities | ||
export { TestPluginStore } from './testing/TestPluginStore'; | ||
export { mockPluginManifest, mockPluginEntryModule } from './testing/mocks'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT - maybe we could rename this directory test-fixtures
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer mocks
since it's more specific to test-fixtures
😃
I always thought that in programming, test fixtures refer to means for providing a fixed (stable, repeatable) testing env. like specific test input data, specific mock objects etc.
But the above functions mockPluginManifest
and mockPluginEntryModule
create new mock objects from provided data with reasonable defaults, I don't think they are technically fixtures since they are not fully fixed, if that makes sense.
import type { PluginManifest } from '../types/plugin'; | ||
import type { PluginEntryModule } from '../types/runtime'; | ||
|
||
export const mockPluginManifest = ({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are these 2 utilities something we really want to expose/maintain? I'd imagine most apps/plugins consuming the SDK and writing tests will be providing their own test manifest/entry module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good question.
Mocking a plugin manifest (like in mockPluginManifest
) is very simple so this could be offloaded to consumers.
Mocking an entry module (like in mockPluginEntryModule
) is a bit more complex, but still most typically a simple transformation from static pluginModules: { [moduleRequest: string]: AnyObject }
declaration to SDK expected get
function (moduleRequest: string) => Promise<() => AnyObject>
- I don't think it makes sense to test asynchronicity within that function, so the resulting Promise would most typically be Promise.resolve(() => pluginModules[moduleRequest])
or similar.
Overall, I think you're right here. We don't have to expose these functions, creating such mock objects should be relatively easy from consumer perspective.
webpackConfig, | ||
}, | ||
specPattern: 'src/components/**/*.cy.{js,jsx,ts,tsx}', | ||
indexHtmlFile: 'src/cypress/component-index.html', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT: Wondering if it makes sense to put the cypress
and e2e
directories in the src
folder or if we should move them out to top level directories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We currently put all the source code in src
directory per each package.
I don't see much value in creating sibling source directories scoped to specific tools or purposes - having all the sources under one directory seems simpler to me. (Test code is source code too, but oftentimes get treated as second class citizen in the codebase.)
AFAK there is no special treatment necessary for src/cypress
directory - this code is used in the Cypress component testing webpack dev build, so the usual context applies (React code to be part of the resulting web application).
I've placed component-index.html
into the cypress
directory to co-locate the HTML template with associated setup code. This is similar to src/app-index.html.ejs
and src/app-styles.css
sitting next to src/app.tsx
.
Note to self: app.css
could be inlined to app-styles.css
and the latter renamed to app.css
.
@@ -25,6 +25,7 @@ | |||
"copy-webpack-plugin": "^10.2.4", | |||
"css-loader": "^6.7.1", | |||
"css-minimizer-webpack-plugin": "^3.4.1", | |||
"cypress": "^12.17.3", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity - any reason we shouldn't bump to 13.x.x
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I posted this PR, Cypress 13 wasn't released yet 😃
yarn npm info cypress -f dist-tags
shows latest: '13.2.0'
which is better than 13.0.x
.
I will check https://docs.cypress.io/guides/references/changelog but generally I'd prefer staying at stable v12 for now.
208865c
to
fa61968
Compare
/retest |
fa61968
to
066a043
Compare
@vojtechszocs: This pull request references RHCLOUD-27198 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
@vojtechszocs: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've checked these changes out locally and ran the sample app/plugin, builds, and tests. Things appear to be functioning as expected.
@vojtechszocs: This pull request references RHCLOUD-27198 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.15.0" version, but no target version was set. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: florkbr, vojtechszocs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Summary of changes:
TestPluginStore
for React components that consume plugin informationPluginLoader
implementation toPluginStore
via thePluginLoaderInterface
PluginInfoTable
showing the number of extensions brought in by the given pluginNotes on Cypress component testing
We recommend using a single
TestPluginStore
instance for all your tests. This can be done via the corresponding Cypress support file which is typically used for one-time preparation of your testing environment.In your component test, you can then just
cy.mount
the given component and usecy.getPluginStore
command to access theTestPluginStore
instance.Check out this article to understand the sync / async duality of Cypress and how it transforms and runs your tests.