-
Notifications
You must be signed in to change notification settings - Fork 603
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
Add ModelFeatureFlag and improve plugin support #1528
Add ModelFeatureFlag and improve plugin support #1528
Conversation
@vojtechszocs: GitHub didn't allow me to request PR reviews from the following users: jelkosz. Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
Follow-up to #1499 |
5bfe1c3
to
8dbd09f
Compare
/cc @alecmerdler |
@@ -0,0 +1,104 @@ | |||
import { |
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.
Why aren't we following the module.spec.ts
filename pattern 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.
You're right 👍 we should follow the existing pattern.
I'll do the change and update Jest testRegex
value accordingly.
@alecmerdler @spadgett What's your opinion on unit test co-location? Right now, the test sits next to the corresponding module. An alternative is to have e.g.
packages/console-plugin-sdk/__tests__/codegen/index.spec.ts
which adds structural duplicity in favor of better test isolation.
Once we move code out of the public
directory into appropriate packages, which test location convention should we follow?
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 prefer having tests next to their source file, but that's not what we had when I started working on bridge
, so we've stuck with the __tests__
directory. We should stick with the duplicate structure; following the module.spec.ts
filename format makes it easy to find the test files using search anyway.
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.
Why not just follow the typical pattern found and described by most react apps and tutorials wherein the __tests__
directory is located adjacent to the file being tested. So in this case create test file at src/codegen/__tests__/index.spec.ts
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 prefer having tests next to their source file
I have the same preference, the main reason being that it's immediately clear if the given module has a corresponding test, and the secondary reason being that it encourages developers to treat tests in the same way as actual code.
I'm OK with @christianvogt's proposal, co-locating tests under __tests__
adjacent to the module being tested. Should I proceed with this change? @spadgett @alecmerdler
The underscores in __tests__
is something I don't like, though. I guess this comes from Python, which uses __
prefix to denote things that are outside of the regular code. (From my point of view, unit tests should be an integral part of the regular code, not something outside of it.)
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.
If we are going to co-locate the tests, my vote would to have a flat structure and not use __tests__
. So:
components/
|--- index.ts
|--- index.spec.ts
|--- widget.tsx
|--- widget.spec.tsx
Whatever we choose, we should do it across the entire codebase to ensure consistency, which means breaking out the existing __tests__
directory to their respective source 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.
Whatever we choose, we should do it across the entire codebase to ensure consistency
I'll take care of that as part of the "moving stuff out of public
directory" effort.
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 agree that we should do it across the whole repo. But do so in steps.
One benefit to the __tests__
dir is that related test files can also be placed in there as to not confused with application code. The double underscores help move the dir to the top of the file sort order. I can appreciate having tests as sibling files though.
When using jest we also have a __mocks__
folder for mocks and a __snapshots__
folder for snapshots. Snapshots could probably be co-located once again in the same dir. TBH I never looked at changing the directory for mocks though.
Something worth bringing up as we look to improve and enhance the coding guidelines as well as look to add more linting.
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.
One benefit to the
__tests__
dir is that related test files can also be placed in there as to not confused with application code. The double underscores help move the dir to the top of the file sort order. I can appreciate having tests as sibling files though.
These are good points. @spadgett do you have any thoughts on this?
For this PR, I'll use the adjacent __tests__
directory convention:
path/to/module.ts(x)
path/to/__tests__/module.spec.ts(x)
When using jest we also have a
__mocks__
folder for mocks and a__snapshots__
folder for snapshots.
Actually, some files under the __mocks__
directory are setup files:
"setupFiles": [
"./__mocks__/requestAnimationFrame.js",
"./__mocks__/localStorage.ts",
"./__mocks__/matchMedia.js",
"./before-tests.js"
],
Some other files in that directory are automatic module mocks:
"moduleNameMapper": {
"\\.(jpg|jpeg|png|gif|eot|otf|webp|svg|ttf|woff|woff2|mp4|webm|wav|mp3|m4a|aac|oga)$": "<rootDir>/__mocks__/fileMock.js",
"\\.(css|less)$": "<rootDir>/__mocks__/styleMock.js"
},
The remaining files in that directory are what I call helper mocks. These are used in tests to provide mocked versions of specific objects, for example:
// in __tests__/components/catalog.spec.tsx
import { catalogListPageProps, catalogItems, catalogCategories } from '../../__mocks__/catalogItemsMocks';
As far as I can see, Console doesn't use Jest manual mocks which are used to stub out entire modules, both application and vendor.
Snapshots could probably be co-located once again in the same dir. TBH I never looked at changing the directory for mocks though.
Jest originally introduced the __snapshots__
directory for the purpose of completely unambiguous test vs. snapshot file resolution.
As of Jest 24, it's possible to customize the test snapshot location by providing a custom snapshot resolver.
I don't see any value in having many __snapshots__
directories dispersed across the codebase, so I'd vote for co-locating test snapshot right next to the test itself. If we go with the adjacent __tests__
convention, this means:
path/to/module.ts(x)
path/to/__tests__/module.spec.ts(x)
path/to/__tests__/module.spec.snap
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.
Also, by putting test snapshots next to the corresponding tests,
related test files can also be placed in there as to not confused with application code
makes even more sense.
@@ -0,0 +1,104 @@ | |||
import { |
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.
Why not just follow the typical pattern found and described by most react apps and tutorials wherein the __tests__
directory is located adjacent to the file being tested. So in this case create test file at src/codegen/__tests__/index.spec.ts
frontend/package.json
Outdated
], | ||
"testRegex": "/__tests__/.*\\.(ts|tsx|js|jsx)$", | ||
"testRegex": "(/__tests__/.*|\\.test)\\.(ts|tsx|js|jsx)$", |
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.
Stick to using spec
as that's the adopted file name pattern
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.
Agreed, will do.
8dbd09f
to
24ceaad
Compare
Rebased & updated according to review comments. |
24ceaad
to
7e24417
Compare
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.
Looks good. Thanks for adding the unit tests. We should open JIRA issues to track remaining work such as moving the tests.
} | ||
|
||
// TODO(vojtech): add ActionBasedFeatureFlag | ||
export type FeatureFlag = ModelBasedFeatureFlag; |
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.
Maybe just ModelFeatureFlag
and ActionFeatureFlag
?
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.
Sounds good, I'll do the change.
An ActionFeatureFlag
should result in adding new function to featureActions
array in public/features.ts
module.
Also, not sure if we need to extend ssarChecks
which are related to featureActions
.
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'll probably want something like an RBACFeatureFlag
eventually. I've been investigating ways of handling RBAC in console generally to conditionally disable actions and nav items. I think we can add it when we need it.
7e24417
to
52b6ec9
Compare
PR updated 👨💻 all review comments should be addressed. |
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.
/lgtm
/retest |
1 similar comment
/retest |
I'm on it. |
52b6ec9
to
e712229
Compare
@spadgett @alecmerdler Currently, there's So I've updated the second one with
and removed the first one 😃 |
e712229
to
b277051
Compare
Fixed the failing |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: spadgett, 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 |
New extensions
This PR adds an extension type that allows Console plugins to contribute new model-based feature flags.
For example:
In general, every Console plugin should contribute one (or possibly more) feature flags related to that plugin. Consequently, any extension of Console UI should be gated by the corresponding feature flag at runtime.
So if e.g. KubeVirt is not detected on the cluster, any KubeVirt related extensions will not be effective.
Code improvements
@console/active-plugins
codegen simplified and unit-testedproperties
types, for example:eslint-plugin-import
bumped to v2.17.2 in order to support TypeScript namespaces<N> plugins active
log whenNODE_ENV
istest
(avoid test output clutter)/cc @spadgett @christianvogt @jelkosz