Fabo/1509 crash tests on warnings

[faboweb]

Closes #1509

Description:

readded the console.error and console.warn throw patches. but also added a quick way to opt out of them.

❤️ Thank you!

---

• Added entries in CHANGELOG.md with issue # and GitHub username
• Reviewed Files changed in the github PR explorer

[jbibla]

why do you want to add this back? it caused issues for us... maybe add a flag to opt in instead?

[NodeGuy]

why do you want to add this back? it caused issues for us... maybe add a flag to opt in instead?

Strong agreement. This caused problems before but you haven't yet demonstrated a benefit for adding it back in. So far the two examples we've discussed (Encrypting the state failed, removing cached state. and the contract test keys.add) are correctly functioning tests that would break with this change.

I prefer that you discuss changes like this (undoing previously discussed and agreed upon work) with the rest of the team before writing code.

[faboweb]

My approach as discussed on Slack:
Apart from the two tests stated, there were warnings in TableValidators, where the used mock data didn't fit the components requirements and nobody noticed/cared.
I disagree that errors/warnings in the console is an acceptable behavior in the tests. Having them acceptable hides real problems. I would like us to have a safety net.
If you know, that your test will produce an error output you can always mock console.error / console.warning for this individual test.
The problem in the past was, that in the case of a console.error, you couldn't identify where it was called from so you would need to comment all the code out. I simplified this process for you so that you just need to call ALLOW_CONSOLE=true jest {you file name}. I thought about using the CI flag that is automatically set on the CI, so that only the CI runs conservative tests. But from experience in this team, it is better to have the local tests run exactly as the ones on the CI.

[jbibla]

I disagree that errors/warnings in the console is an acceptable behavior in the tests

i think i agree with you here @faboweb - can the tests fail without obscuring the logs though?

If you know, that your test will produce an error output you can always mock console.error / console.warning

is there a situation where the expected behaviour is a console error?

it is better to have the local tests run exactly as the ones on the CI.

yep, i agree.

[faboweb]

can the tests fail without obscuring the logs though?

I didn't fine a possibility to do that.

is there a situation where the expected behaviour is a console error?

yes in the case of the failing contract test David mentioned

[codecov]

Codecov Report

Merging #1542 into develop will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff            @@
##           develop    #1542   +/-   ##
========================================
  Coverage    96.62%   96.62%           
========================================
  Files          100      100           
  Lines         1953     1953           
  Branches        95       95           
========================================
  Hits          1887     1887           
  Misses          56       56           
  Partials        10       10

Impacted Files	Coverage Δ
...pp/src/renderer/components/staking/LiValidator.vue	96% <ø> (ø)	⬆️
...rc/renderer/components/staking/TableValidators.vue	97.22% <ø> (ø)	⬆️

[fedekunze]

utACK