Feat show testplane config in ui - #806
Conversation
27f793c to
acd5867
Compare
✅ Component tests succeed
|
acd5867 to
320a644
Compare
commit: |
✅ E2E tests succeed
|
953d4ff to
98d3f1a
Compare
98d3f1a to
434bb7a
Compare
shadowusr
left a comment
shadowusr
left a comment
There was a problem hiding this comment.
Exceptionally wonderful and long-awaited feature! 🔥
I've listed a couple of issues in comments, though, that would be nice to fix.
| this._config = TestplaneConfigAdapter.create(this._tool.config); | ||
| this._browserConfigs = _.map(this._config.browserIds, (id) => this._config.getBrowserConfig(id)); | ||
| this._htmlReporter = HtmlReporter.create(this._reporterConfig, {toolName: ToolName.Testplane}); | ||
| this._htmlReporter.testplaneConfig = this._config.getUserConfig(); |
There was a problem hiding this comment.
Why do we name this field testplaneConfig? I see that currently we support only Testplane, but nothing stops us of adding this feature to other tools as well. I would give it a generic name, like toolConfig or userConfig, etc.
There was a problem hiding this comment.
Fixed, toolConfig now
| hasCloseButton={true} | ||
| onClose={(): void => setIsOpen(false)} | ||
| > | ||
| <Dialog.Header caption={'Testplane config'}/> |
There was a problem hiding this comment.
This results in an inconsistent text style, let's use either panel heading style or section heading style:
| import type {ConfigAdapter} from './'; | ||
| import type {TestplaneTestAdapter} from '../test/testplane'; | ||
|
|
||
| export const maskTokenValues = (value: unknown): unknown => { |
There was a problem hiding this comment.
Hmm, I see a pretty important issue here:
- we only look at the field name over here, "token", specifically
- if user has sensitive data in any other field, they have absolutely zero way to fix that
So, with this update, we unconditionally expose sensitive data in all fields not named "token" and user has no way to fix this.
I see two ways of improving this:
-
Look not only at the field name, but at the contents, too, for example strings containing
y1__,AQAD, etc. -
Provide a way for users to customize this behavior, either turn it off completely or provide their own sanitizer. We should think hard, what's best in this case: trying to avoid adding too many difficult to use options yet providing an escape hatch if user needs it.
We can also look how other providers solve this, for example, pwt.
And this 100% should be tested on large projects, to check that at least for them, it works out of the box.
There was a problem hiding this comment.
Fixed, added new func secretConfigFilter, user can configure it, but by default used internal filter for keys token, secret etc...
| return this._config.forBrowser(browserId); | ||
| } | ||
|
|
||
| getUserConfig(): Record<string, unknown> { |
There was a problem hiding this comment.
I think configPath should not be displayed as part of the config object. Because it's not actually in user config, it's a side-effect of how we work with config. But it's actually a good idea to display it as a separate field in the dialog!
There was a problem hiding this comment.
Fixed, now separate filed in ui
| } | ||
|
|
||
| // eslint-disable-next-line @typescript-eslint/no-var-requires | ||
| const configModule: unknown = require(path.resolve(this._config.configPath)); |
There was a problem hiding this comment.
Hmmm, this discards all env and CLI args overrides.
In fact, simply return maskTokenValues(this._config.serialize()) as Record<string, unknown>; gives a more correct config, doesn't it?
It gives a fully resolved config, with all values filled, including unchanged defaults. Yes, it's pretty long, but probably much more useful? And we could post-process it to make it smaller, for example, by omitting values inside browser specifications if they match root level values, etc.
We can discuss in a team chat, if you want.
There was a problem hiding this comment.
First I did it, but config with default values too big, and as I think we want see only user config
| </Button> | ||
| </div> | ||
| <div className={classNames(styles.config, {[styles.breakLines]: breakLines})}> | ||
| <JsonNode value={config} depth={0} areAllNestedExpanded={areAllNestedExpanded}/> |
There was a problem hiding this comment.
While I don't suggest fixing it right away, but it's just sad that we have 3 different code block styles across the report: this one, error stack traces, and critical error stack traces :(
At some point, it would be nice to unify them.
|
Also, let's add at least a couple of e2e tests on this new feature. They should be honest, representing how users would interact with the app. For example open config and check that it contains expected values. |
8d98f73 to
2844683
Compare
a8134ad to
09da993
Compare
2 participants
Now you can see testplane config in Info section of sidebar menu.