Skip to content

Conversation

BacLuc
Copy link
Contributor

@BacLuc BacLuc commented Feb 21, 2021

No description provided.

Also rename the test that it uses the correct component name.

Issue: ecamp#412
That we can remove the id field so that it does not
change constantly.

Issue: ecamp#412
Because it contains id's and they change.

Issue: ecamp#412
@BacLuc BacLuc force-pushed the frontend-add-tests-api-select branch from fb26309 to f9bf9cf Compare February 21, 2021 14:07
@BacLuc BacLuc force-pushed the frontend-add-tests-api-select branch from f9bf9cf to 1a1e33e Compare February 21, 2021 14:28
Comment on lines +1 to +87
function mockPromiseResolving (value) {
return new Promise((resolve, reject) => {
const timer = setTimeout(() => {
clearTimeout(timer)
resolve(value)
}, 100)
})
}

class MockStubbing {
constructor (fieldName, value) {
this._fieldName = fieldName
this._value = value
}

forFieldName (fieldName) {
this._fieldName = fieldName
return this
}

get fieldName () {
return this._fieldName
}

get value () {
return this._value
}
}

class ApiMockState {
constructor () {
this._get = jest.fn()
this._patch = jest.fn()
}

getMocks () {
return {
get: this._get,
patch: this._patch
}
}

get () {
const apiMock = this
return {
thenReturn (mockStubbing) {
if (!(mockStubbing instanceof MockStubbing)) {
throw new Error('apiMock must be instance of MockStubbing')
}
if (mockStubbing.fieldName === undefined || mockStubbing.value === undefined) {
throw new Error('fieldName and value must be defined')
}
apiMock._get.mockReturnValue({
[mockStubbing.fieldName]: mockStubbing.value,
_meta: {
load: Promise.resolve(mockStubbing.value)
}
})
}
}
}

patch () {
const apiMock = this
return {
thenReturn (mockStubbing) {
if (!(mockStubbing instanceof MockStubbing)) {
throw new Error('apiMock must be instance of MockStubbing')
}
if (mockStubbing.fieldName !== undefined || mockStubbing.value === undefined) {
throw new Error('fieldName must be undefined and value must be defined')
}
apiMock._patch.mockReturnValue(mockPromiseResolving(mockStubbing.value))
}
}
}
}

export class ApiMock {
static create () {
return new ApiMockState()
}

static success (value) {
return new MockStubbing(undefined, value)
}
}
Copy link
Member

@carlobeltrame carlobeltrame Feb 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow, that's a lot of code for adding just a single test. This looks like something we could integrate directly in hal-json-vuex, would you agree? I am worried that this will be hard to maintain if it's sitting here.

Is there a special reason why this is necessary here and not in the other ApiFormComponent tests?

Copy link
Contributor Author

@BacLuc BacLuc Feb 24, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I started here and thought i don't want this massive object in the test just to mock the api.
For the next tests, my live will be easier.

It would be nice if hal-json-vuex would offer a way to mock itself in tests.
I would wait a little to look if it really helps to write shorter tests with less boilerplate, and then we can migrate it to hal-json-vuex.
I also did not check if my mocks represent the behaviour of hal-json-vuex correctly, it worked for that test.

With JUnit you normally just write the hierarchy in the test like:
const api = jest.fn()
const get = jest.fn()
api.mockReturnValue('get', get)
get.mockReturnValue

Maybe there is a reason to not write such a mocking helper, but to repeat yourself?

Copy link
Member

@carlobeltrame carlobeltrame left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Als temporärer Zustand finde ichs okay zum mergen. Längerfristig sollte der Test nochmals umgeschrieben werden und ApiMock.js entfernt werden, wenn hal-json-vuex das von Haus aus kann.

const timer = setTimeout(() => {
clearTimeout(timer)
resolve(value)
}, 100)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Warum reichen 100 Millisekunden? Was garantiert uns dass wir das nicht beim nächsten Test auf 200 Millisekunden hochschrauben müssen? Könnte man auch 0 Millisekunden nehmen? In JavaScript ist ein Timeout von 0 Millisekunden ausreichend, um auf die nächste Runde des Event Loops zu warten. Vielleicht brauchts auch gar keinen Timeout?

return new Promise((resolve) => {
  resolve(value)
})

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ist von ApiWrapper kopiert

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ja, habe ich inzwischen auch gesehen. Das sollte dann wenn möglich auch dort abgelöst werden.

@BacLuc BacLuc requested review from usu, simfeld and manuelmeister March 13, 2021 14:43
@pmattmann pmattmann merged commit 1a1e33e into ecamp:devel Mar 23, 2021
@BacLuc BacLuc deleted the frontend-add-tests-api-select branch November 2, 2021 14:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants