Ensure that Jest testRegex catches all the specs #2204

vojtechszocs · 2019-07-29T15:56:16Z

Fixes CONSOLE-1658

The change in Jest testRegex made in commit b4bc8c9 caused some specs not to be executed.

Files under __tests__ are either actual tests or test helpers (utils, mocks, etc). Actual tests should have spec in their filename. We want to co-locate test helpers with actual tests, but we also want Jest to not match those helpers.

$ NON_SPEC_FILES=`find . -type f -regex '.*/__tests__/.*' | grep -vF 'node_modules' | grep -vF '.spec.'`
$ grep -l 'describe' $NON_SPEC_FILES 
./__tests__/components/utils/datetime.ts
./__tests__/public/co-fetch.js
./__tests__/resource.js
./__tests__/selector-requirement.js
./__tests__/selector.js
./__tests__/container.ts
./__tests__/probe.ts
./__tests__/units.js

Above ^^ files have been git-mv'ed to contain spec in their filename.

frontend/__tests__/units.spec.js is still failing, so using xdescribe as a temporary fix. I've talked with @rawagner and he'll take a closer look on that.

cc @jelkosz

spadgett

/lgtm

spadgett · 2019-07-29T16:02:31Z

We'll see if the tests still pass... :/

vojtechszocs · 2019-07-29T16:06:05Z

There are some test failures, I'll fix them.

christianvogt · 2019-07-29T16:21:27Z

frontend/package.json

@@ -43,7 +43,7 @@
    "transformIgnorePatterns": [
      "<rootDir>/node_modules/(?!(lodash-es|@console|@novnc|@spice-project)/.*)"
    ],
-    "testRegex": "/__tests__/.*\\.spec\\.(ts|tsx|js|jsx)$",
+    "testRegex": "/__tests__/.*\\.(ts|tsx|js|jsx)$",


This can't be right. Don't we need spec in there because for dev-console we put non-test files inside the __tests__ dir as support utils or data.

Exactly, we need this for all the monorepo tests, otherwise it blows up on test helpers (and rightly so).

$ yarn test packages .. packages/dev-console/src/components/pipelines/__tests__/pipeline-mock.ts .. Your test suite must contain at least one test.

We could use Jest projects feature, or just git-mv the 8 files in frontend/__tests__ to correctly follow the spec convention.

@vojtechszocs just make testRegex an array. One for packages and one for everything else.

@vojtechszocs actually just move the files to the correct format and be done with it. It's where we want to go anyways.

openshift-bot · 2019-07-29T16:34:44Z

/retest

Please review the full test history for this PR and help us cut down flakes.

vojtechszocs · 2019-07-29T17:03:31Z

PR updated, git-mv'ed above mentioned files to ensure they are matched by Jest.

spadgett · 2019-07-29T18:20:29Z

/retest

spadgett · 2019-07-29T20:46:39Z

frontend/__tests__/units.spec.js

@@ -2,7 +2,7 @@ import * as _ from 'lodash-es';

 import { units, validate, convertToBaseValue } from '../public/components/utils/units';

-describe('units', () => {
+xdescribe('units', () => {


We should really try to fix these tests. How bad are the failures?

@rawagner has fixed the tests 🎉

He said the problem originates from commit b14d83b (change in round function).

vojtechszocs · 2019-07-30T17:58:22Z

/retest

spadgett · 2019-07-30T18:36:44Z

/assign @TheRealJon

vojtechszocs · 2019-08-01T13:09:50Z

@christianvogt @alecmerdler @TheRealJon

Tests are fixed now, can you please take a look if you have some time?

christianvogt · 2019-08-01T16:43:23Z

Seems ok, but unsure of the history and ux expectation for the unit formatting.

frontend/public/components/utils/units.js

spadgett · 2019-08-07T15:24:01Z

frontend/public/components/utils/units.js

  const {locales, ...rest} = _.defaults(options, {
-    maximumFractionDigits: fractionDigits,
+    maximumFractionDigits: 20,


What is the reason for this change? 20 seems like a big value, but maybe I'm misunderstanding this option.

fraction digits are determined by round function

console/frontend/public/components/utils/units.js

Line 140 in 21c5ba2

const multiplier = Math.pow(10, fractionDigits || getDefaultFractionDigits(value));

and formatValue function is used only for formatting string part - I didnt want to change value here so I set maximumFractionDigits to maximum that is allowed

Do we need round at all if we can use maximumFractionDigits for formatting here?

This should at least have a comment explaining.

humanize functions return object

value: number; unit: string; string: string;

at first we work on value -> it gets rounded (result is number type) and then the rounded value is passed to formatValue which is using Intl.NumberFormat.format and the result is string type.

I we use only Intl.NumberFormat.format then we would need to parse the resulting string back to number which seems a more clumsy to me.

I will add a comment explaining this if we decide to go with current approach

So I'm still unclear why we need to change this implementation here. Is this fixing a bug, changing behavior, or just refactoring? Can you give an example of the before / after with this change?

Based on your description, I think the existing API is a bit confusing. I would expect

A function that takes a string value with units and returns an object like {value, unit}

A function that takes the base, unitless value, picks the best unit, and formats it using Intl.NumberFormat and specifying significant digits

A function that takes {value, unit} and formats it using Intl.NumberFormat and specifying significant digits

I'm not proposing we make these changes in this PR, but we might look at refactoring this down the road.

If we keep the fractionDigits in Intl.NumberFormat config and call humanize with useRound=false we will end up with value not rounded, but string rounded. Maybe that is not an issue, useRound=false isnt even used anywhere in code right now and when we refactor things in the future we will either way end up with something different.

If we keep the fractionDigits in Intl.NumberFormat config and call humanize with useRound=false we will end up with value not rounded, but string rounded.

This is exactly what I would expect :) I think it's a better API. Is this what the code was doing before?

Ok :) Reverted the change. Before we started using Intl.NumberFormat both value and string was the same (string was just value + space + unit), after Intl.NumberFormat was introduced the things got different/tests started to fail so I wasnt sure if this change was on purpose or not. But I agree that it makes sense to keep the logic as is.

OK, I see the complaint now. It's the the string value is rounded when useRound = false. Sorry I misunderstood.

I think the fix is to always round the humanized value and just remove useRound if it's not used anywhere today. But that can be a follow on.

Fix units tests

spadgett

/approve
/lgtm

spadgett · 2019-08-15T21:09:14Z

Looks like we have one test failure:

FAIL __tests__/units.spec.js (91.846s)
  ● units › should humanize numeric values › 0.1234 into 0.1234

spadgett · 2019-08-15T21:10:56Z

/lgtm cancel
as we have a test failure

rawagner · 2019-08-16T07:02:06Z

sorry about that, i forgot to push latest changes, should be fine now.

rawagner · 2019-08-16T10:48:58Z

/test e2e-aws-console

spadgett

/lgtm
/approve

openshift-ci-robot · 2019-08-16T12:15:40Z

[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:

~~frontend/OWNERS~~ [spadgett,vojtechszocs]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 29, 2019

openshift-ci-robot requested review from alecmerdler and benjaminapetersen July 29, 2019 15:56

spadgett approved these changes Jul 29, 2019

View reviewed changes

openshift-ci-robot assigned spadgett Jul 29, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2019

vojtechszocs mentioned this pull request Jul 29, 2019

Use bits instead of bytes for network measurements #2161

Closed

christianvogt reviewed Jul 29, 2019

View reviewed changes

Ensure that Jest testRegex catches all the specs

7524302

vojtechszocs force-pushed the revert-testregex branch from 8e7b3f3 to 7524302 Compare July 29, 2019 16:53

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2019

vojtechszocs changed the title ~~Revert to original Jest testRegex~~ Ensure that Jest testRegex catches all the specs Jul 29, 2019

spadgett reviewed Jul 29, 2019

View reviewed changes

Fix units tests

f385db6

openshift-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. component/core Related to console core functionality and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jul 30, 2019

openshift-ci-robot assigned TheRealJon Jul 30, 2019

rawagner mentioned this pull request Aug 7, 2019

Adding the missing Y-axis labels to NooBaa-dashboard's Data Consumption card #2253

Merged

spadgett reviewed Aug 7, 2019

View reviewed changes

spadgett mentioned this pull request Aug 15, 2019

Bug 1741136: Use 1 decimal place for values < 100, 0 otherwise #2361

Closed

rawagner force-pushed the revert-testregex branch from 21c5ba2 to d749c18 Compare August 15, 2019 13:03

Merge pull request #1 from rawagner/fix_units_tests

25dbfce

Fix units tests

spadgett approved these changes Aug 15, 2019

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2019

spadgett added this to the v4.2 milestone Aug 15, 2019

openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Aug 15, 2019

rawagner force-pushed the revert-testregex branch from d749c18 to 25dbfce Compare August 16, 2019 06:28

openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 16, 2019

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Aug 16, 2019

spadgett approved these changes Aug 16, 2019

View reviewed changes

openshift-merge-robot merged commit 9dc137f into openshift:master Aug 16, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure that Jest testRegex catches all the specs #2204

Ensure that Jest testRegex catches all the specs #2204

vojtechszocs commented Jul 29, 2019 •

edited

Loading

spadgett left a comment

spadgett commented Jul 29, 2019

vojtechszocs commented Jul 29, 2019

christianvogt Jul 29, 2019 •

edited

Loading

vojtechszocs Jul 29, 2019

christianvogt Jul 29, 2019

christianvogt Jul 29, 2019

openshift-bot commented Jul 29, 2019

vojtechszocs commented Jul 29, 2019

spadgett commented Jul 29, 2019

spadgett Jul 29, 2019

vojtechszocs Jul 30, 2019

vojtechszocs commented Jul 30, 2019

spadgett commented Jul 30, 2019

vojtechszocs commented Aug 1, 2019 •

edited

Loading

christianvogt commented Aug 1, 2019

spadgett Aug 7, 2019

rawagner Aug 7, 2019

spadgett Aug 7, 2019

rawagner Aug 7, 2019

spadgett Aug 15, 2019

rawagner Aug 15, 2019

spadgett Aug 15, 2019

rawagner Aug 15, 2019 •

edited

Loading

spadgett Aug 15, 2019

spadgett left a comment

spadgett commented Aug 15, 2019

spadgett commented Aug 15, 2019

rawagner commented Aug 16, 2019

rawagner commented Aug 16, 2019

spadgett left a comment

openshift-ci-robot commented Aug 16, 2019

Ensure that Jest testRegex catches all the specs #2204

Ensure that Jest testRegex catches all the specs #2204

Conversation

vojtechszocs commented Jul 29, 2019 • edited Loading

spadgett left a comment

Choose a reason for hiding this comment

spadgett commented Jul 29, 2019

vojtechszocs commented Jul 29, 2019

christianvogt Jul 29, 2019 • edited Loading

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

openshift-bot commented Jul 29, 2019

vojtechszocs commented Jul 29, 2019

spadgett commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vojtechszocs commented Jul 30, 2019

spadgett commented Jul 30, 2019

vojtechszocs commented Aug 1, 2019 • edited Loading

christianvogt commented Aug 1, 2019

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

rawagner Aug 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spadgett left a comment

Choose a reason for hiding this comment

spadgett commented Aug 15, 2019

spadgett commented Aug 15, 2019

rawagner commented Aug 16, 2019

rawagner commented Aug 16, 2019

spadgett left a comment

Choose a reason for hiding this comment

openshift-ci-robot commented Aug 16, 2019

vojtechszocs commented Jul 29, 2019 •

edited

Loading

christianvogt Jul 29, 2019 •

edited

Loading

vojtechszocs commented Aug 1, 2019 •

edited

Loading

rawagner Aug 15, 2019 •

edited

Loading