-
Notifications
You must be signed in to change notification settings - Fork 272
chore: coordinate superset-ui unittest with main repository #1463
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/superset/superset-ui/HP3YmmXBKokKqvxjV9yq8HN4DQiM |
Codecov Report
@@ Coverage Diff @@
## master #1463 +/- ##
==========================================
+ Coverage 30.91% 30.94% +0.02%
==========================================
Files 503 502 -1
Lines 10215 10213 -2
Branches 1761 1761
==========================================
+ Hits 3158 3160 +2
+ Misses 6805 6801 -4
Partials 252 252
Continue to review full report at Codecov.
|
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.
LGTM, but I'd prefer to get @kgabryje @geido or @michael-s-molina to also take a look, as this is unfamiliar territory to me.
const spy = jest.spyOn(logging, 'warn'); | ||
|
||
beforeAll(() => { | ||
spy.mockImplementation((info: any) => { |
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 explain why do we need to mock logging methods? And why do they throw errors?
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 have removed @airbnb/config-jest/setup/console.js
from jest.config.js
. this script makes console.warn
return a mock function.(This logic is simple though. But it stuck me for a long time.)
So I'm thinking of using the normal mock function in the unit test is better than before.
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.
Oh, I see, so we just bring back relevant changes from removed library 👍
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.
There are some test cases that need to get warning message.
|
||
// to support: npx jest --silent | ||
const spy = jest.spyOn(logging, 'trace'); | ||
spy.mockImplementation(() => { |
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.
Should we include logging.trace
's argument here? Like:
throw new Error('Trace: ' + traceMessage)
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 mock just for logging.trace()
, Ensure that when call the logging.trace()
, there is Trace:
at the beginning of the return.
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.
Looks good, but just like Ville I'm not 100% familiar with those configs. Let's have third pair of eyes take a look just to be sure
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.
lgtm
🏠 Internal
coordinate superset-ui unittest with main repository
@airbnb/config-jest
@airbnb/config-jest
related "setup" before unit test