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
kubevirt: add tests for Wrappers, EnhancedK8sMethods and utility classes #4696
kubevirt: add tests for Wrappers, EnhancedK8sMethods and utility classes #4696
Conversation
@@ -0,0 +1,12 @@ | |||
import { Wrapper } from '../../wrapper'; |
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.
this file is a simple example of a Wrapper
@@ -0,0 +1,85 @@ | |||
import { ObjectEnum } from '../../../../../constants'; |
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.
this file is a simple example of ObjectWithTypePropertyWrapper
129be38
to
c4c4271
Compare
/retest |
1 similar comment
/retest |
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.
Great tests @suomiy
if (_.isEmpty(this.getDataVolumeTemplates())) { | ||
delete this.data.spec.dataVolumeTemplates; | ||
} | ||
this.clearIfEmpty('spec.template.spec.domain.devices.disks'); |
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.
can't we merge ensureStorages
and ensureStorageConsistency
into one method?
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.
no, one is called before the change of data occurs and the other is called after the change of data - please see prependStorage
method
await methods.k8sPatch(VirtualMachineModel, testVM, [testPatch]); | ||
const actualState = methods.getActualState(); | ||
|
||
expect(actualState).toHaveLength(2); |
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.
can you destruct actualState
for readability? or add a comment? I have no idea whats inside.
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.
added a better comment to the getActualState
method
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.
and returns K8sResourceKind[]
expect(methodsUnchecked.history).toHaveLength(0); | ||
|
||
await methods.k8sCreate(VirtualMachineModel, testVM); | ||
expect(methods.getHistory()).toHaveLength(1); |
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.
can you swap the expects? so first we check that there is history and then we actually test that there isn't
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.
the other way is better - otherwise it could happend that some object overwrites the first one we checked and we wouldn't know a difference
const results = await methods.rollback(); | ||
|
||
expect(results).toHaveLength(2); | ||
expect(methods.getActualState()).toHaveLength(0); |
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 is getActualState()
empty? just to understand..
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.
because the rollback worked and there are no living objects we created before on the cluster
blueberry?: { | ||
color?: string; | ||
}; | ||
strawberry?: { |
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.
where is 🍍 ?
😅
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.
~>.<
expect(emptySmoothie.hasType()).toBe(false); | ||
expect(emptySmoothie.asResource()).toEqual({}); | ||
}); | ||
// |
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.
empty comment
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.
fixed
}); | ||
}); | ||
|
||
it('stores inconsistent data', () => { |
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 don't understand this test, can you write more informative comments? thanks
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.
added a comment few lines bellow
@@ -4,7 +4,7 @@ import { apiVersionForModel, K8sKind, K8sResourceKind } from '@console/internal/ | |||
import { Wrapper } from './wrapper'; | |||
import { K8sResourceKindMethods } from '../types/types'; | |||
|
|||
export class K8sResourceWrapper< | |||
export abstract class K8sResourceWrapper< |
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 we sure there is no use case of creating an instance of K8sResourceWrapper
?
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.
It is better if the user creates its own definition if needed. Now it also requires model to be supplied.
@@ -2,7 +2,16 @@ import * as _ from 'lodash'; | |||
|
|||
export const omitEmpty = (obj, justUndefined = false) => { |
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.
isn't there a lodash
alternative so we can remove this code?
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.
have not found one
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.
Creates an array with all falsey values removed. The values
false
,null
,0
,""
,undefined
, andNaN
are falsey.
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.
Alternatively, you could use _.remove
or _.without
.
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.
Cannot be used very well. It should work on objects and arrays recursively with an option to switch between null x undefined
c4c4271
to
314004c
Compare
added few changes to the wrappers since last time
|
also added mixins for our two diverged wrappers - just the more complicated methods for now |
314004c
to
fc77dd6
Compare
methodsUnchecked.registerKind(kind); | ||
methodsUnchecked.appendHistory(new HistoryItem(HistoryType.DELETE, data), enhancedOpts); | ||
}; | ||
return { methods, methodsUnchecked }; |
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 do we need to separate these two? Both should reference the same object.
Is methodsUnchecked
meant to avoid further as any
casts?
We could create a minimal type for methodsUnchecked
(based on code that uses it) and return methods
typed as e.g. BaseK8sMethods & EnhancedK8sMethods
.
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.
made a different method in the spyEnhancedK8sMethods
methods.mockK8sGet to avoid the type incompatibilities
using just methods outside of this one and using any typecasts
const expectHistory = (history, expectedHistory) => { | ||
expect(history).toHaveLength(expectedHistory.length); | ||
|
||
expectedHistory.forEach((expectHistoryItem, idx) => { |
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.
IIUC the history objects are one-dimensional arrays.
If so, isn't expect(history).toEqual(expectedHistory)
enough?
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 are right - this is much easier to read. It was meant to print an index which failed, but I guess it is not that useful.
}) | ||
.build(); | ||
|
||
it('records history and shows actualState', async () => { |
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.
it('records history and shows actualState', async () => { | |
it('records history', async () => { |
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.
changed
}); | ||
}; | ||
|
||
describe('enhancedK8sMethods.js', () => { |
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.
describe('enhancedK8sMethods.js', () => { | |
describe('enhancedK8sMethods', () => { |
I'd use the logical module name to describe the test suite.
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.
done
expect.assertions(4); | ||
let rollbackError; | ||
try { | ||
await methods.rollback(); |
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 could use the toThrowError
matcher here.
expect(methods.rollback).toThrowError('rollback');
// alternatively perform equality check against provided object
expect(methods.rollback).toThrowError(new Error('rollback'));
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.
This does not work very well as the error must implement Constructable
frontend/packages/kubevirt-plugin/src/k8s/wrapper/common/util/k8s-mixin.ts
Show resolved
Hide resolved
frontend/packages/kubevirt-plugin/src/k8s/wrapper/utils/__tests__/utils.spec.ts
Outdated
Show resolved
Hide resolved
frontend/packages/kubevirt-plugin/src/k8s/wrapper/utils/utils.ts
Outdated
Show resolved
Hide resolved
@@ -2,7 +2,16 @@ import * as _ from 'lodash'; | |||
|
|||
export const omitEmpty = (obj, justUndefined = false) => { |
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.
Creates an array with all falsey values removed. The values
false
,null
,0
,""
,undefined
, andNaN
are falsey.
@@ -2,7 +2,16 @@ import * as _ from 'lodash'; | |||
|
|||
export const omitEmpty = (obj, justUndefined = false) => { |
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.
Alternatively, you could use _.remove
or _.without
.
fc77dd6
to
1e224e5
Compare
fixed |
/retest |
- add k8sWrapperCreate method to EnhancedK8sMethods
1e224e5
to
d4b54a4
Compare
added deprecated statements to all initializeFromSimpleData functions |
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: suomiy, 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 |
also fixes few bugs which were found by the testing
@vojtechszocs @glekner @irosenzw @mareklibra @yaacov @pcbailey