[ML] Transforms: Refactor validators and add unit tests. #173736

walterra · 2023-12-20T12:48:15Z

Summary

Part of #173301.

Splits up form validators and adds more unit tests.

Checklist

Unit or functional tests were updated or added to match the most common scenarios
This was checked for breaking API changes and was labeled appropriately

elasticmachine · 2023-12-21T09:01:55Z

Pinging @elastic/ml-ui (:ml)

x-pack/plugins/transform/public/app/common/validators/frequency_validator.test.ts

x-pack/plugins/transform/public/app/common/validators/integer_range_10_to_10000_validator.ts

darnautov · 2023-12-22T14:04:35Z

...ins/transform/public/app/common/validators/transform_settings_number_of_retries_validator.ts

+);
+
+export const transformSettingsNumberOfRetriesValidator: Validator = (value) =>
+  !(value + '').includes('.') &&


Can this be a part of a generic validator?

darnautov · 2023-12-22T14:04:40Z

...gins/transform/public/app/common/validators/transform_settings_page_search_size_validator.ts

+ * @param value User input value.
+ */
+export const transformSettingsPageSearchSizeValidator: Validator = (value) =>
+  !(value + '').includes('.') &&


Can this be a part of a generic validator?

darnautov · 2023-12-22T14:05:40Z

x-pack/plugins/transform/public/app/common/validators/string_validator.ts

+
+import type { Validator } from './types';
+
+export const stringValidator: Validator = (value, isOptional = true) => {


we already have a generic requiredValidator

darnautov · 2023-12-22T14:08:09Z

...ck/plugins/transform/public/app/common/validators/retention_policy_max_age_validator.test.ts

+  const transformRetentionPolicyMaxAgeValidator = (arg: string) =>
+    retentionPolicyMaxAgeValidator(arg).length === 0;


I reckon we should not write unit tests for functions created in the test suites itself

Fixed in 0193145.

darnautov · 2023-12-22T14:13:37Z

x-pack/plugins/transform/public/app/common/validators/is_valid_frequency.ts

+import type { ParsedDuration } from './types';
+
+// only valid if value is up to 1 hour
+export function isValidFrequency(arg: unknown): arg is ParsedDuration {


It would be useful to have a generic duration validator, e.g.

const getDurationValidator = ({ minDurationInMs: number; maxDurationInMs: number; allowedUnits: stiring[] })

x-pack/plugins/transform/public/app/common/validators/is_json_string.ts

darnautov · 2023-12-22T14:21:31Z

x-pack/plugins/transform/public/app/common/validators/is_continuous_mode_delay.ts

+ * Doesn't allow floating intervals.
+ * @param value User input value.
+ */
+export function isContinuousModeDelay(value: string): boolean {


we have a generic patternValidator

Looks like patternValidator lives in the ml plugin, not transform. We could at one point create a package for validators and share that among plugins, but I'm afraid it's not something I'm going to pick up in this PR 😅 .

darnautov · 2023-12-22T14:23:31Z

x-pack/plugins/transform/public/app/common/validators/integer_range_minus_1_to_100_validator.ts

+export const integerRangeMinus1To100Validator: Validator = (value) =>
+  !(value + '').includes('.') &&
+  numberValidator({ min: -1, max: 100, integerOnly: true })(+value) === null
+    ? []
+    : [numberRangeMinus1To100NotValidErrorMessage];


can we use a generic validator I mentioned before?
another important moment to consider, depending on the locale, a comma might be used as a decimal separator

darnautov · 2023-12-22T14:24:27Z

x-pack/plugins/transform/public/app/common/validators/integer_above_zero_validator.ts

+);
+
+export const integerAboveZeroValidator: Validator = (value) =>
+  !(value + '').includes('.') && numberValidator({ min: 1, integerOnly: true })(+value) === null


Can we use a generic validator and avoid creating a function for each specific case?

I'm not sure I fully get the suggestion. This is making use of the generic validator numberValidator, it then sets this up as a wrapper for a specific use case required by the edit transform flyout to satisfy the spec of the request object to update a transform. Also note this isn't new code, all the validators have just been moved to individual files and a corresponding jest test file, the intent of this PR wasn't to change any logic but just to spread out the existing validators into separate files. Can you share more details about how you envision that generic validator?

Why do you need the !(value + '').includes('.') part? The generic validator already checks for integers with integerOnly.

We have a test assertion in there that would fail if remove that part: expect(integerAboveZeroValidator('1.')).toEqual(['Value needs to be an integer above zero.']);. I guess it's because 1. technically passes Number.isInteger but we didn't want users to be able to enter 1..

darnautov · 2023-12-22T14:24:31Z

x-pack/plugins/transform/public/app/common/validators/integer_range_minus_1_to_100_validator.ts

+  }
+);
+
+export const integerRangeMinus1To100Validator: Validator = (value) =>


Can we use a generic validator and avoid creating a function for each specific case?

walterra · 2024-01-02T10:23:42Z

@darnautov I addressed your comments and adjusted the test assertions. As mentioned in one of the comments, I'd like to avoid a larger refactor in this PR, the aim was just to split up larger files into files with 1 validator and corresponding test file. Please have another look.

…a/kibana into ml-transform-refactor-validators

walterra · 2024-01-02T12:19:22Z

@darnautov As discussed, I removed the checks for !(value + '').includes('.') in 49f1284

kibana-ci · 2024-01-02T15:06:46Z

💚 Build Succeeded

Buildkite Build
Commit: 1270555

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`transform`	392	405	+13

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
`transform`	403.4KB	402.8KB	-632.0B

Unknown metric groups

ESLint disabled line counts

id	before	after	diff
`transform`	33	32	-1

Total ESLint disabled count

id	before	after	diff
`transform`	37	36	-1

History

💔 Build #185385 failed 02c5998
💚 Build #185368 succeeded ed1f5f2
💚 Build #184922 succeeded 66c160e
💔 Build #184914 failed 90e20ac
💚 Build #184601 succeeded 83d4d13

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @walterra

alvarezmelissa87

LGTM ⚡

* main: (4129 commits) [Logs Explorer] Change the default link for "Discover" in the serverless nav (#173420) [Fleet] fix unhandled error in agent details when components are missing (#174152) [Obs UX] Unskip transaction duration alerts test (#174069) [Fleet] Fix keep policies up to date after package install (#174093) [Profiling] Stack traces embeddable (#173905) [main] Sync bundled packages with Package Storage (#174115) [SLO Form] Refactor to use kibana data view component (#173513) [Obs UX] Unskip APM Service Inventory Journey (#173510) [Obs UX] Unskip preview_chart_error_count test (#173624) [api-docs] 2024-01-03 Daily api_docs build (#174142) Update babel runtime helpers (#171745) Handle content stream errors in report pre-deletion (#173792) [Cloud Posture] [Quick wins] Enable edit DataView on the Misconfigurations Findings Table (#173870) [ftr] abort retry on invalid webdriver session (#174092) Upgrade openai to 4.24.1 (#173934) chore(NA): bump node into v20 (#173461) [Security Solution][Endpoint] Fix index name pattern in SentinelOne dev. script (#174105) fix versions.json [Obs AI Assistant] Add guardrails (#174060) [ML] Transforms: Refactor validators and add unit tests. (#173736) ...

walterra self-assigned this Dec 20, 2023

refactor validators

83d4d13

walterra force-pushed the ml-transform-refactor-validators branch from 9d549df to 83d4d13 Compare December 20, 2023 15:16

walterra added :ml Feature:Transforms ML transforms v8.13.0 release_note:skip Skip the PR/issue when compiling release notes labels Dec 21, 2023

walterra marked this pull request as ready for review December 21, 2023 09:01

walterra requested a review from a team as a code owner December 21, 2023 09:01

walterra requested review from darnautov and alvarezmelissa87 December 21, 2023 09:03

darnautov reviewed Dec 21, 2023

View reviewed changes

x-pack/plugins/transform/public/app/common/validators/frequency_validator.test.ts Outdated Show resolved Hide resolved

darnautov reviewed Dec 21, 2023

View reviewed changes

x-pack/plugins/transform/public/app/common/validators/integer_range_10_to_10000_validator.ts Outdated Show resolved Hide resolved

walterra added 4 commits December 22, 2023 11:50

Merge branch 'main' into ml-transform-refactor-validators

63662a5

remove helper function

0e8ab71

remove unused validators

90e20ac

fix i18n

66c160e

darnautov reviewed Dec 22, 2023

View reviewed changes

walterra added 4 commits January 2, 2024 10:15

Merge branch 'main' into ml-transform-refactor-validators

737fc6a

don't log json encoding error

eb68c06

assert validator messages

0193145

add test case for isJsonString(true)

5321409

[CI] Auto-commit changed files from 'node scripts/notice'

ed1f5f2

peteharverson mentioned this pull request Jan 2, 2024

[ML] Increase Test Coverage 8.13.0 #173301

Closed

11 tasks

walterra added 3 commits January 2, 2024 13:03

relax integer validation

49f1284

fix typo

13a8114

Merge branch 'ml-transform-refactor-validators' of github.com:walterr…

02c5998

…a/kibana into ml-transform-refactor-validators

darnautov approved these changes Jan 2, 2024

View reviewed changes

fix memoized validator

1270555

alvarezmelissa87 approved these changes Jan 2, 2024

View reviewed changes

walterra merged commit 34935a2 into elastic:main Jan 2, 2024
19 checks passed

kibanamachine added the backport:skip This commit does not require backporting label Jan 2, 2024

walterra deleted the ml-transform-refactor-validators branch January 2, 2024 18:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ML] Transforms: Refactor validators and add unit tests. #173736

[ML] Transforms: Refactor validators and add unit tests. #173736

walterra commented Dec 20, 2023 •

edited

Loading

elasticmachine commented Dec 21, 2023

darnautov Dec 22, 2023

darnautov Dec 22, 2023

darnautov Dec 22, 2023

darnautov Dec 22, 2023

walterra Jan 2, 2024

darnautov Dec 22, 2023

darnautov Dec 22, 2023

walterra Dec 22, 2023

darnautov Dec 22, 2023

darnautov Dec 22, 2023

walterra Dec 22, 2023

darnautov Jan 2, 2024 •

edited

Loading

walterra Jan 2, 2024

darnautov Dec 22, 2023

walterra commented Jan 2, 2024

walterra commented Jan 2, 2024

kibana-ci commented Jan 2, 2024

ESLint disabled line counts

Total ESLint disabled count

alvarezmelissa87 left a comment


		import type { Validator } from './types';

		export const stringValidator: Validator = (value, isOptional = true) => {

		const transformRetentionPolicyMaxAgeValidator = (arg: string) =>
		retentionPolicyMaxAgeValidator(arg).length === 0;

[ML] Transforms: Refactor validators and add unit tests. #173736

[ML] Transforms: Refactor validators and add unit tests. #173736

Conversation

walterra commented Dec 20, 2023 • edited Loading

Summary

Checklist

elasticmachine commented Dec 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

darnautov Jan 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

walterra commented Jan 2, 2024

walterra commented Jan 2, 2024

kibana-ci commented Jan 2, 2024

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

ESLint disabled line counts

Total ESLint disabled count

History

alvarezmelissa87 left a comment

Choose a reason for hiding this comment

walterra commented Dec 20, 2023 •

edited

Loading

darnautov Jan 2, 2024 •

edited

Loading