Skip to content
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

fix(resource): add if condition (DSP-1655) #448

Merged
merged 1 commit into from May 25, 2021
Merged

Conversation

@kilchenmann
Copy link
Collaborator

@kilchenmann kilchenmann commented May 21, 2021

resolves DSP-1655

@kilchenmann kilchenmann self-assigned this May 21, 2021
@kilchenmann kilchenmann requested a review from mdelez May 21, 2021
@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented May 21, 2021

@mdelez What do you think about this quick solution?

Loading

@mdelez
Copy link
Contributor

@mdelez mdelez commented May 21, 2021

In what scenario do you find yourself in a situation where a ValueAdded event is raised but there is no value?

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented May 21, 2021

In what scenario do you find yourself in a situation where a ValueAdded event is raised but there is no value?

only in the test... yes maybe we should fix it in the test. But I have no clue about this setup. The test failed because "newValue" is undefined: TypeError: Cannot read property 'addedValue' of undefined

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented May 25, 2021

@mdelez Maybe I'll need your help here. Any suggestion?

Loading

@mdelez
Copy link
Contributor

@mdelez mdelez commented May 25, 2021

@mdelez Maybe I'll need your help here. Any suggestion?

I can take a look at it later today

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented May 25, 2021

Maybe @flavens has also an idea!?

Loading

@mdelez
Copy link
Contributor

@mdelez mdelez commented May 25, 2021

Strangely, if I run npm run test-ci locally, I get a different error message. I can't get the same ValueAdded error 🤔 What happens when you run this command locally? No errors?

Screen Shot 2021-05-25 at 11 47 11

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented May 25, 2021

Strangely, if I run npm run test-ci locally, I get a different error message. I can't get the same ValueAdded error 🤔 What happens when you run this command locally? No errors?

Yes, this is the strange thing. I do not have an error, but sometimes I have the same error as on Github CI. But sometimes I also had your error message, but test were successful. This is what happened right now on main branch...

Loading

@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented May 25, 2021

On Github CI the tests failed on the mentioned error Cannot read property 'addedValue' of undefined, but when re-running all the tests, they are successful. But this could not be the solution, right?

Loading

@mdelez
Copy link
Contributor

@mdelez mdelez commented May 25, 2021

On Github CI the tests failed on the mentioned error Cannot read property 'addedValue' of undefined, but when re-running all the tests, they are successful. But this could not be the solution, right?

This is definitely a solution but I feel like we should figure out why this is actually happening at some point. My guess is that it's a timing issue. There is probably an async BeforeEach somewhere that is not ready in time sometimes. For now, I'll approve it.

Loading

mdelez
mdelez approved these changes May 25, 2021
@kilchenmann
Copy link
Collaborator Author

@kilchenmann kilchenmann commented May 25, 2021

On Github CI the tests failed on the mentioned error Cannot read property 'addedValue' of undefined, but when re-running all the tests, they are successful. But this could not be the solution, right?

This is definitely a solution but I feel like we should figure out why this is actually happening at some point. My guess is that it's a timing issue. There is probably an async BeforeEach somewhere that is not ready in time sometimes. For now, I'll approve it.

Yes absolutely, I agree.

Loading

@kilchenmann kilchenmann merged commit 656da04 into main May 25, 2021
8 checks passed
Loading
@kilchenmann kilchenmann deleted the wip/dsp-1655-fix-unit-tests branch May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants